diff mbox

[4/7,ARC,LRA] Avoid emitting COND_EXEC during expand.

Message ID 1496324097-21221-5-git-send-email-claziss@synopsys.com
State New
Headers show

Commit Message

Claudiu Zissulescu June 1, 2017, 1:34 p.m. UTC
Emmitting COND_EXEC rtxes during expand does not always work.

gcc/
2017-01-10  Claudiu Zissulescu  <claziss@synopsys.com>

	* config/arc/arc.md (clzsi2): Expand to an arc_clzsi2 instruction
	that also clobbers the CC register. The old expand code is moved
	to ...
	(*arc_clzsi2): ... here.
	(ctzsi2): Expand to an arc_ctzsi2 instruction that also clobbers
	the CC register. The old expand code is moved to ...
	(arc_ctzsi2): ... here.
---
 gcc/config/arc/arc.md | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

Comments

Andrew Burgess July 13, 2017, 11:38 a.m. UTC | #1
* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2017-06-01 15:34:54 +0200]:

> Emmitting COND_EXEC rtxes during expand does not always work.
> 
> gcc/
> 2017-01-10  Claudiu Zissulescu  <claziss@synopsys.com>
> 
> 	* config/arc/arc.md (clzsi2): Expand to an arc_clzsi2 instruction
> 	that also clobbers the CC register. The old expand code is moved
> 	to ...
> 	(*arc_clzsi2): ... here.
> 	(ctzsi2): Expand to an arc_ctzsi2 instruction that also clobbers
> 	the CC register. The old expand code is moved to ...
> 	(arc_ctzsi2): ... here.

This seems fine, your description "....does not always work." is a bit
of a tease :) it would be nice to know _why_ it doesn't work, or at
least a description of what problem you're seeing.

Also we seem to be missing a test, would it be possible to find one?
If not then I guess we live without, but we should note that in the
commit message.

Thanks,
Andrew




> ---
>  gcc/config/arc/arc.md | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
> index 39bcc26..928feb1 100644
> --- a/gcc/config/arc/arc.md
> +++ b/gcc/config/arc/arc.md
> @@ -4533,9 +4533,21 @@
>     (set_attr "type" "two_cycle_core,two_cycle_core")])
>  
>  (define_expand "clzsi2"
> -  [(set (match_operand:SI 0 "dest_reg_operand" "")
> -	(clz:SI (match_operand:SI 1 "register_operand" "")))]
> +  [(parallel
> +    [(set (match_operand:SI 0 "register_operand" "")
> +	  (clz:SI (match_operand:SI 1 "register_operand" "")))
> +     (clobber (match_dup 2))])]
> +  "TARGET_NORM"
> +  "operands[2] = gen_rtx_REG (CC_ZNmode, CC_REG);")
> +
> +(define_insn_and_split "*arc_clzsi2"
> +  [(set (match_operand:SI 0 "register_operand" "=r")
> +	(clz:SI (match_operand:SI 1 "register_operand" "r")))
> +   (clobber (reg:CC_ZN CC_REG))]
>    "TARGET_NORM"
> +  "#"
> +  "reload_completed"
> +  [(const_int 0)]
>  {
>    emit_insn (gen_norm_f (operands[0], operands[1]));
>    emit_insn
> @@ -4552,9 +4564,23 @@
>  })
>  
>  (define_expand "ctzsi2"
> -  [(set (match_operand:SI 0 "register_operand" "")
> -	(ctz:SI (match_operand:SI 1 "register_operand" "")))]
> +  [(match_operand:SI 0 "register_operand" "")
> +   (match_operand:SI 1 "register_operand" "")]
>    "TARGET_NORM"
> +  "
> +  emit_insn (gen_arc_ctzsi2 (operands[0], operands[1]));
> +  DONE;
> +")
> +
> +(define_insn_and_split "arc_ctzsi2"
> +  [(set (match_operand:SI 0 "register_operand" "=r")
> +	(ctz:SI (match_operand:SI 1 "register_operand" "r")))
> +   (clobber (reg:CC_ZN CC_REG))
> +   (clobber (match_scratch:SI 2 "=&r"))]
> +  "TARGET_NORM"
> +  "#"
> +  "reload_completed"
> +  [(const_int 0)]
>  {
>    rtx temp = operands[0];
>  
> @@ -4562,10 +4588,10 @@
>        || (REGNO (temp) < FIRST_PSEUDO_REGISTER
>  	  && !TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS],
>  				 REGNO (temp))))
> -    temp = gen_reg_rtx (SImode);
> +    temp = operands[2];
>    emit_insn (gen_addsi3 (temp, operands[1], constm1_rtx));
>    emit_insn (gen_bic_f_zn (temp, temp, operands[1]));
> -  emit_insn (gen_clrsbsi2 (temp, temp));
> +  emit_insn (gen_clrsbsi2 (operands[0], temp));
>    emit_insn
>      (gen_rtx_COND_EXEC
>        (VOIDmode,
> @@ -4575,7 +4601,8 @@
>      (gen_rtx_COND_EXEC
>        (VOIDmode,
>         gen_rtx_GE (VOIDmode, gen_rtx_REG (CC_ZNmode, CC_REG), const0_rtx),
> -       gen_rtx_SET (operands[0], gen_rtx_MINUS (SImode, GEN_INT (31), temp))));
> +       gen_rtx_SET (operands[0], gen_rtx_MINUS (SImode, GEN_INT (31),
> +						operands[0]))));
>    DONE;
>  })
>  
> -- 
> 1.9.1
>
Claudiu Zissulescu July 13, 2017, 12:54 p.m. UTC | #2
> This seems fine, your description "....does not always work." is a bit
> of a tease :) it would be nice to know _why_ it doesn't work, or at
> least a description of what problem you're seeing.
> 
As far as I can see, LRA doesn't handle very well the conditional execution patterns, as it expects conditional execution to happen after this step. Thus, some of those instructions are marked dead and removed later on.

> Also we seem to be missing a test, would it be possible to find one?
> If not then I guess we live without, but we should note that in the
> commit message.

This error is found by executing dg.exp testsuite with our port and -mlra option on.  As we speak, I am on the last 100m of testing our port having the LRA on. This bug being found like that.

I'll add this discussion to the commit message body.

Thank you,
Claudiu
Claudiu Zissulescu July 17, 2017, 1:01 p.m. UTC | #3
> This seems fine, your description "....does not always work." is a bit
> of a tease :) it would be nice to know _why_ it doesn't work, or at
> least a description of what problem you're seeing.

Committed with additional comments. Thank you, 
Claudiu
diff mbox

Patch

diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md
index 39bcc26..928feb1 100644
--- a/gcc/config/arc/arc.md
+++ b/gcc/config/arc/arc.md
@@ -4533,9 +4533,21 @@ 
    (set_attr "type" "two_cycle_core,two_cycle_core")])
 
 (define_expand "clzsi2"
-  [(set (match_operand:SI 0 "dest_reg_operand" "")
-	(clz:SI (match_operand:SI 1 "register_operand" "")))]
+  [(parallel
+    [(set (match_operand:SI 0 "register_operand" "")
+	  (clz:SI (match_operand:SI 1 "register_operand" "")))
+     (clobber (match_dup 2))])]
+  "TARGET_NORM"
+  "operands[2] = gen_rtx_REG (CC_ZNmode, CC_REG);")
+
+(define_insn_and_split "*arc_clzsi2"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+	(clz:SI (match_operand:SI 1 "register_operand" "r")))
+   (clobber (reg:CC_ZN CC_REG))]
   "TARGET_NORM"
+  "#"
+  "reload_completed"
+  [(const_int 0)]
 {
   emit_insn (gen_norm_f (operands[0], operands[1]));
   emit_insn
@@ -4552,9 +4564,23 @@ 
 })
 
 (define_expand "ctzsi2"
-  [(set (match_operand:SI 0 "register_operand" "")
-	(ctz:SI (match_operand:SI 1 "register_operand" "")))]
+  [(match_operand:SI 0 "register_operand" "")
+   (match_operand:SI 1 "register_operand" "")]
   "TARGET_NORM"
+  "
+  emit_insn (gen_arc_ctzsi2 (operands[0], operands[1]));
+  DONE;
+")
+
+(define_insn_and_split "arc_ctzsi2"
+  [(set (match_operand:SI 0 "register_operand" "=r")
+	(ctz:SI (match_operand:SI 1 "register_operand" "r")))
+   (clobber (reg:CC_ZN CC_REG))
+   (clobber (match_scratch:SI 2 "=&r"))]
+  "TARGET_NORM"
+  "#"
+  "reload_completed"
+  [(const_int 0)]
 {
   rtx temp = operands[0];
 
@@ -4562,10 +4588,10 @@ 
       || (REGNO (temp) < FIRST_PSEUDO_REGISTER
 	  && !TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS],
 				 REGNO (temp))))
-    temp = gen_reg_rtx (SImode);
+    temp = operands[2];
   emit_insn (gen_addsi3 (temp, operands[1], constm1_rtx));
   emit_insn (gen_bic_f_zn (temp, temp, operands[1]));
-  emit_insn (gen_clrsbsi2 (temp, temp));
+  emit_insn (gen_clrsbsi2 (operands[0], temp));
   emit_insn
     (gen_rtx_COND_EXEC
       (VOIDmode,
@@ -4575,7 +4601,8 @@ 
     (gen_rtx_COND_EXEC
       (VOIDmode,
        gen_rtx_GE (VOIDmode, gen_rtx_REG (CC_ZNmode, CC_REG), const0_rtx),
-       gen_rtx_SET (operands[0], gen_rtx_MINUS (SImode, GEN_INT (31), temp))));
+       gen_rtx_SET (operands[0], gen_rtx_MINUS (SImode, GEN_INT (31),
+						operands[0]))));
   DONE;
 })