This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns
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.