[DAG] Check for preexisting store when emiting stack convert

[niravd]

Summary

Before creating a new store to do a cast, check if an existing store is viable
and can be used.

[jlebar]

This seems sane to me, but I don't quite feel comfortable signing off on it.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1664

see if we

1665

location. If so, use that.

1666

No need for a blank line here, since the comment applies just to the loop.

test/CodeGen/PowerPC/share-bitcast-stores.ll

4

a viable one

4

Please wrap to 80 chars.

[jyknight]

Is it actually okay to introduce a read from a memory location that didn't have one in the source? This might do the wrong thing if e.g. if you have two threads non-atomically writing to a global, but never reading it. I'd suspect it's not okay, because it introduces a data race, but I'm not certain.

Going back to the case this is supposed to be improving, from ppc64-align-long-double.ll in the D14834 patch. In that case, there *was* a load, but it got eliminated as unnecessary, by forwarding the value through from the store, instead. So, by the time we realize that the bitcast needs to be lowered to store then load, that information is gone.

We could change this patch to only allow introducing new loads from a spill slot -- that might handle the particular thing noticed in the above test, and not have issues with potential for incorrectness. But that seems a somewhat small special case so I'm not sure it's actually worth it. I mean -- the calling lowering of that function is already bad, with seemingly-spurious storing of registers to memory in lowering the struct* byval argument, anyways.

I'm inclined to suggest just reverting this and leaving that test with its FIXME...

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

 }
 
 /// Emit a store/load combination to the stack.  This stores
 /// SrcOp to a stack slot of type SlotVT, truncating it if needed.  It then does
 /// a load from the stack slot to DestVT, extending it if needed.
 /// The resultant code need not be legal.
 SDValue SelectionDAGLegalize::EmitStackConvert(SDValue SrcOp, EVT SlotVT,
                                                EVT DestVT, const SDLoc &dl) {
-  // Create the stack frame object.
+
+  unsigned SrcSize = SrcOp.getValueSizeInBits();
+  unsigned SlotSize = SlotVT.getSizeInBits();
+  unsigned DestSize = DestVT.getSizeInBits();
+  Type *DestType = DestVT.getTypeForEVT(*DAG.getContext());
   unsigned SrcAlign = DAG.getDataLayout().getPrefTypeAlignment(
       SrcOp.getValueType().getTypeForEVT(*DAG.getContext()));
-  SDValue FIPtr = DAG.CreateStackTemporary(SlotVT, SrcAlign);
+  unsigned DestAlign = DAG.getDataLayout().getPrefTypeAlignment(DestType);
+
+  // Before we allocate space on the stack, see we already have an
+  // equivalent store to another location and leverage that.
+
+  for (SDNode *U : SrcOp->uses()) {
+    if (MemSDNode *SNode = dyn_cast<MemSDNode>(U))
+      if (SNode->getOpcode() == ISD::STORE && SNode->getOperand(1) == SrcOp &&
+          SlotVT.bitsEq(SNode->getMemoryVT())) {
+        if (SlotSize == DestSize)
+          return DAG.getLoad(DestVT, dl, SDValue(SNode, 0),
+                             SNode->getOperand(2), SNode->getPointerInfo(),
+                             DestAlign);
+        assert(SlotSize < DestSize && "Unknown extension!");
+        return DAG.getExtLoad(ISD::EXTLOAD, dl, DestVT, SDValue(SNode, 0),
+                              SNode->getOperand(2), SNode->getPointerInfo(),
+                              SlotVT, DestAlign);
+      }
+  }
 
+  // Create the stack frame object.
+  SDValue FIPtr = DAG.CreateStackTemporary(SlotVT, SrcAlign);
   FrameIndexSDNode *StackPtrFI = cast<FrameIndexSDNode>(FIPtr);
   int SPFI = StackPtrFI->getIndex();
   MachinePointerInfo PtrInfo =
       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), SPFI);
 
-  unsigned SrcSize = SrcOp.getValueSizeInBits();
-  unsigned SlotSize = SlotVT.getSizeInBits();
-  unsigned DestSize = DestVT.getSizeInBits();
-  Type *DestType = DestVT.getTypeForEVT(*DAG.getContext());
-  unsigned DestAlign = DAG.getDataLayout().getPrefTypeAlignment(DestType);
 
   // Emit a store to the stack slot.  Use a truncstore if the input value is
   // later than DestVT.
   SDValue Store;
 
   if (SrcSize > SlotSize)
     Store = DAG.getTruncStore(DAG.getEntryNode(), dl, SrcOp, FIPtr, PtrInfo,
                               SlotVT, SrcAlign);

test/CodeGen/PowerPC/share-bitcast-stores.ll

+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -O0 -fast-isel=false -mattr=-vsx < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -O0 -fast-isel=false -mattr=-vsx < %s | FileCheck -check-prefix=CHECKSAFE %s
+
+; Verify that bitcasts realized as store-load pairs don't emit extra stores if viable one already exists. 
+
+target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
+target triple = "powerpc64-unknown-linux-gnu"
+
+@number64 = global i64 10, align 8
+
+define double @f64toi64(i64 %a) {
+; We should never see a negative offset store
+; CHECKSAFE-LABEL: f64toi64
+; CHECKSAFE-NOT:   std {{[0-9]+}, -{{.*}}
+; CHECK-LABEL:     f64toi64
+; CHECK:     std 3, 0(4)
+; CHECK-NEXT:      lfd 1, 0(4)
+entry:
+  store i64 %a, i64* @number64
+  %0 = bitcast i64 %a to double
+  ret double %0
+}