[LLVMdev] Is r174746 broken on ARM?

[Hal Finkel]

----- Original Message -----
> From: "Dmitry Antipov" <antipov at dev.rtsoft.ru>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Renato Golin" <renato.golin at linaro.org>, llvmdev at cs.uiuc.edu
> Sent: Monday, April 8, 2013 2:45:20 AM
> Subject: Re: Is r174746 broken on ARM?
> 
> On 04/04/2013 05:09 PM, Hal Finkel wrote:
> 
> > Looking briefly at the code in comment 5 of PR15581, is that the
> > pre-decrement case?
> > I can't test that case on PPC, so I can certainly believe that
> > there is a problem somewhere.
> > The relevant code is a little farther down:
> >
> >      APInt OV =
> >        cast<ConstantSDNode>(Offset)->getAPIntValue();
> >      if (AM == ISD::PRE_DEC)
> >        OV = -OV;
> >
> >      ConstantSDNode *CN =
> >        cast<ConstantSDNode>(OtherUses[i]->getOperand(OffsetIdx));
> >      APInt CNV = CN->getAPIntValue();
> >      if (OtherUses[i]->getOpcode() == ISD::SUB && OffsetIdx == 1)
> >        CNV += OV;
> >      else
> >        CNV -= OV;
> >
> > perhaps something here is not quite right.
> 
> I suspect that the first snippet (where OV is inverted) is wrong
> because
> ARM implementation of getPreIndexedAddressParts inverts Offset for
> pre-decrement case, both for ARM and Thumb2, in
> getARMIndexedAddressParts
> and getT2IndexedAddressParts, respectively, in a calls to:
> 
> Offset = DAG.getConstant(-RHSC, RHS->getValueType(0));
>                           ^^^^^
>                           here!
> 
> So you don't need to invert it one more time in
> DAGCombiner::CombineToPreIndexedLoadStore.

Okay, great! Can you please generate a test case? In case you don't know, an easy way is this: place an assert that will crash the code generation in the pre-dec case you've highlighted, then use bugpoint to produce a reduced test case for the crash.

Thanks again,
Hal

> 
> Dmitry
> 
> 
>