<div dir="ltr"><div>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. </div><div><br></div><div>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. </div><div><br></div><div>In any case, my inclination is to leave this as is for now. </div><div><br></div><div>-Nirav</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 24, 2017 at 4:08 PM, Friedman, Eli <span dir="ltr"><<a href="mailto:efriedma@codeaurora.org" target="_blank">efriedma@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 5/24/2017 12:55 PM, Nirav Dave via llvm-commits wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: niravd<br>
Date: Wed May 24 14:55:49 2017<br>
New Revision: 303800<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=303800&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=303800&view=rev</a><br>
Log:<br>
[AArch64] Prevent nested ADDs from address calc in splitStoreSplat. NFC<br>
<br>
In preparation for late-stage store merging.<br>
<br>
Modified:<br>
     llvm/trunk/lib/Target/<wbr>AArch64/AArch64ISelLowering.<wbr>cpp<br>
<br>
Modified: llvm/trunk/lib/Target/AArch64/<wbr>AArch64ISelLowering.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=303800&r1=303799&r2=303800&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/lib/Target/AA<wbr>rch64/AArch64ISelLowering.cpp?<wbr>rev=303800&r1=303799&r2=303800<wbr>&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Target/AArch64/<wbr>AArch64ISelLowering.cpp (original)<br>
+++ llvm/trunk/lib/Target/AArch64/<wbr>AArch64ISelLowering.cpp Wed May 24 14:55:49 2017<br>
@@ -9222,16 +9222,26 @@ static SDValue splitStoreSplat(Selection<br>
    // instructions (stp).<br>
    SDLoc DL(&St);<br>
    SDValue BasePtr = St.getBasePtr();<br>
+  int64_t BaseOffset = 0;<br>
+<br>
    const MachinePointerInfo &PtrInfo = St.getPointerInfo();<br>
    SDValue NewST1 =<br>
        DAG.getStore(St.getChain(), DL, SplatVal, BasePtr, PtrInfo,<br>
                     OrigAlignment, St.getMemOperand()->getFlags()<wbr>);<br>
  +  // As this in ISel, we will not merge this add which may degrate results.<br>
+  if (BasePtr->getOpcode() == ISD::ADD &&<br>
+      isa<ConstantSDNode>(BasePtr->g<wbr>etOperand(1))) {<br>
+    BaseOffset = cast<ConstantSDNode>(BasePtr-><wbr>getOperand(1))->getSExtValue()<wbr>;<br>
+    BasePtr = BasePtr->getOperand(0);<br>
+  }<br>
+<br>
    unsigned Offset = EltOffset;<br>
    while (--NumVecElts) {<br>
      unsigned Alignment = MinAlign(OrigAlignment, Offset);<br>
-    SDValue OffsetPtr = DAG.getNode(ISD::ADD, DL, MVT::i64, BasePtr,<br>
-                                    DAG.getConstant(Offset, DL, MVT::i64));<br>
+    SDValue OffsetPtr = DAG.getNode(<br>
+        ISD::ADD, DL, MVT::i64, BasePtr,<br>
+        DAG.getConstant(BaseOffset + ((int64_t)Offset), DL, MVT::i64));<br>
</blockquote>
<br></div></div>
uint64_t?  (Not likely to matter in practice, but "BaseOffset + Offset" could in theory wrap.)<span class="HOEnZb"><font color="#888888"><br>
<br>
-Eli<br>
<br>
-- <br>
Employee of Qualcomm Innovation Center, Inc.<br>
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<br>
<br>
</font></span></blockquote></div><br></div>