On Wed, 2011-03-30 at 12:38 +0800, Chung-Lin Tang wrote: > On 2011/3/30 06:35 AM, Ramana Radhakrishnan wrote: > > Hi Carrot, > > > > On 26/03/11 15:14, Carrot Wei wrote: > >> Index: arm.md > >> =================================================================== > >> --- arm.md (revision 171337) > >> +++ arm.md (working copy) > >> @@ -7115,7 +7115,18 @@ > >> "@ > >> cmp%?\\t%0, %1 > >> cmn%?\\t%0, #%n1" > >> - [(set_attr "conds" "set")] > >> + [(set_attr "conds" "set") > >> + (set (attr "length") > >> + (if_then_else > >> + (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0)) > >> + (eq (symbol_ref "which_alternative") (const_int 0))) > >> + (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0)) > >> + (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0)) > >> + (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0)) > >> + (le (symbol_ref "INTVAL (operands[1])") > >> + (const_int 255)))))) > >> + (const_int 2) > >> + (const_int 4)))] > >> ) > > > > 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 > > > > > >> > >> (define_insn "*cmpsi_shiftsi" > >> @@ -7286,7 +7297,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)))] > >> ) > > > > 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. > > > > > >> @@ -10256,7 +10288,26 @@ > >> > >> 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 "{ > >> + 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; i++) > > > > > > (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. > > I suggest abstracting all of this Thumb-2 insn length knowledge into a > arm.c function, say "thumb2_16bit_insn_p()" to test for 2-byte patterns. > Adding more of these quite large pieces of logic into arm.md does not > look very pretty to me, and centralizing it into a C function will also > make it easier to manage the cases. > > Chung-Lin >
I think Ramana's suggestion should avoid the need for any arithmetic at all here. If the constraints precisely match a 16-bit instruction (and that alternative is disbled for ARM) then we can say unconditionally that the length of the insn is 2. R.