[llvm] r303800 - [AArch64] Prevent nested ADDs from address calc in splitStoreSplat. NFC

[Nirav Davé via llvm-commits]

Hmm. Wrapping was why I had it signed (I was looking at a negative
BaseOffset). I believe we want the signed interpretation for the constant
here.

That said, it looks like getConstant only accepts a uint64_t so we are
implicitly casting. The internal ConstantInt the value is fed into does
allow it to be interpreted as signed. Presumably we do not have signed
constant nodes to improve constant sharing. I wonder if it's worth
propagating signedness throughout to explicitly avoid any
overflow/underflow problems.

In any case, my inclination is to leave this as is for now.

-Nirav


On Wed, May 24, 2017 at 4:08 PM, Friedman, Eli <efriedma at codeaurora.org>
wrote:

> On 5/24/2017 12:55 PM, Nirav Dave via llvm-commits wrote:
>
>> Author: niravd
>> Date: Wed May 24 14:55:49 2017
>> New Revision: 303800
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=303800&view=rev
>> Log:
>> [AArch64] Prevent nested ADDs from address calc in splitStoreSplat. NFC
>>
>> In preparation for late-stage store merging.
>>
>> Modified:
>>      llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>>
>> Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AA
>> rch64/AArch64ISelLowering.cpp?rev=303800&r1=303799&r2=303800&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp (original)
>> +++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp Wed May 24
>> 14:55:49 2017
>> @@ -9222,16 +9222,26 @@ static SDValue splitStoreSplat(Selection
>>     // instructions (stp).
>>     SDLoc DL(&St);
>>     SDValue BasePtr = St.getBasePtr();
>> +  int64_t BaseOffset = 0;
>> +
>>     const MachinePointerInfo &PtrInfo = St.getPointerInfo();
>>     SDValue NewST1 =
>>         DAG.getStore(St.getChain(), DL, SplatVal, BasePtr, PtrInfo,
>>                      OrigAlignment, St.getMemOperand()->getFlags());
>>   +  // As this in ISel, we will not merge this add which may degrate
>> results.
>> +  if (BasePtr->getOpcode() == ISD::ADD &&
>> +      isa<ConstantSDNode>(BasePtr->getOperand(1))) {
>> +    BaseOffset = cast<ConstantSDNode>(BasePtr->
>> getOperand(1))->getSExtValue();
>> +    BasePtr = BasePtr->getOperand(0);
>> +  }
>> +
>>     unsigned Offset = EltOffset;
>>     while (--NumVecElts) {
>>       unsigned Alignment = MinAlign(OrigAlignment, Offset);
>> -    SDValue OffsetPtr = DAG.getNode(ISD::ADD, DL, MVT::i64, BasePtr,
>> -                                    DAG.getConstant(Offset, DL,
>> MVT::i64));
>> +    SDValue OffsetPtr = DAG.getNode(
>> +        ISD::ADD, DL, MVT::i64, BasePtr,
>> +        DAG.getConstant(BaseOffset + ((int64_t)Offset), DL, MVT::i64));
>>
>
> uint64_t?  (Not likely to matter in practice, but "BaseOffset + Offset"
> could in theory wrap.)
>
> -Eli
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170525/f9ce30fb/attachment.html>