Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns

[Carrot Wei <carrot@google.com>]

Hi Ramana

On Wed, Mar 30, 2011 at 6:35 AM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> Hi Carrot,
>
>
>        How about adding an alternative only enabled for T2 that uses the `l'
> constraint and inventing new constraints for some of the constant values
> that are valid for 16 bit instructions since the `I' and `L' constraints
> have different meanings depending on whether TARGET_32BIT is valid or not ?
> We could then set the value of the length attribute accordingly. I don't
> think we can change the meaning of the I and L constraints very easily given
> the amount of churn that might be needed
>
You are right. Now the logic is much clearer by splitting the constraints.
>
>        I don't think this and the other conditional branch instruction
> changes are correct. This could end up being the last instruction in an IT
> block and will automatically end up getting the 32 bit encoding and end up
> causing trouble with the length calculations. Remember the 16 bit encoding
> for the conditional instruction can't be used as the last instruction in an
> IT block.
>
According to arm architecture reference manual, chapter A8.6.16, neither 16 bit
nor 32 bit conditional branch can be used in IT block. Only unconditional branch
can be used as the last instruction in IT block, and it isn't related
to instruction
length. So we don't need to care about branch instruction encoding in IT block.

>
>        (i < num_saves && (hi_reg == 0)) - what's the point of going through
> the register list when hi_reg != 0 in this case ? A comment to explain that
> the length calculation *must* be kept in sync with the template above might
> be useful.
done.

The following patch has been tested on arm qemu.


ChangeLog:
2011-03-31  Wei Guozhi  <carrot@google.com>

        PR target/47855
        * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length".
        (arm_cond_branch): Likewise.
        (arm_cond_branch_reversed): Likewise.
        (arm_jump): Likewise.
        (push_multi): Likewise.
        * config/arm/constraints.md (Py): New constraint.


Index: constraints.md
===================================================================
--- constraints.md	(revision 171337)
+++ constraints.md	(working copy)
@@ -31,7 +31,7 @@
 ;; The following multi-letter normal constraints have been used:
 ;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dz
 ;; in Thumb-1 state: Pa, Pb, Pc, Pd
-;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px
+;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py

 ;; The following memory constraints have been used:
 ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
@@ -189,6 +189,11 @@
   (and (match_code "const_int")
        (match_test "TARGET_THUMB2 && ival >= -7 && ival <= -1")))

+(define_constraint "Py"
+  "@internal In Thumb-2 state a constant in the range 0 to 255"
+  (and (match_code "const_int")
+       (match_test "TARGET_THUMB2 && ival >= 0 && ival <= 255")))
+
 (define_constraint "G"
  "In ARM/Thumb-2 state a valid FPA immediate constant."
  (and (match_code "const_double")
Index: arm.md
===================================================================
--- arm.md	(revision 171337)
+++ arm.md	(working copy)
@@ -7109,13 +7109,21 @@

 (define_insn "*arm_cmpsi_insn"
   [(set (reg:CC CC_REGNUM)
-	(compare:CC (match_operand:SI 0 "s_register_operand" "r,r")
-		    (match_operand:SI 1 "arm_add_operand"    "rI,L")))]
+	(compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r")
+		    (match_operand:SI 1 "arm_add_operand"    "Py,r,I,L")))]
   "TARGET_32BIT"
   "@
    cmp%?\\t%0, %1
+   cmp%?\\t%0, %1
+   cmp%?\\t%0, %1
    cmn%?\\t%0, #%n1"
-  [(set_attr "conds" "set")]
+  [(set_attr "conds" "set")
+   (set (attr "length")
+     (if_then_else
+       (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+	    (lt (symbol_ref "which_alternative") (const_int 2)))
+       (const_int 2)
+       (const_int 4)))]
 )

 (define_insn "*cmpsi_shiftsi"
@@ -7286,7 +7294,14 @@
   return \"b%d1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*arm_cond_branch_reversed"
@@ -7305,7 +7320,14 @@
   return \"b%D1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )



@@ -7757,7 +7779,14 @@
     return \"b%?\\t%l0\";
   }
   "
-  [(set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -2044))
+		     (le (minus (match_dup 0) (pc)) (const_int 2048))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*thumb_jump"
@@ -10256,7 +10285,29 @@

     return \"\";
   }"
-  [(set_attr "type" "store4")]
+  [(set_attr "type" "store4")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(ne (symbol_ref "{
+		    /* Check if there are any high register (except lr)
+		       references in the list. KEEP the following iteration
+		       in sync with the template above.  */
+		    int i, regno, hi_reg;
+		    int num_saves = XVECLEN (operands[2], 0);
+		    regno = REGNO (operands[1]);
+		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
+			     && (regno != LR_REGNUM);
+		    for (i = 1; i < num_saves && !hi_reg; i++)
+		      {
+			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
+			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
+				  && (regno != LR_REGNUM);
+		      }
+		    !hi_reg;    }")
+		  (const_int 0)))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "stack_tie"