Fix store detection for return value in CGCall

[kuhar]

Summary

findDominatingStoreToReturn in CGCall.cpp didn't check if a candidate store instruction used the ReturnValue as pointer operand or value operand. This led to wrong code gen - in later stages (load-store elision code) the found store and its operand would be erased, causing ReturnValue to become a <badref>.

The patch adds a check that makes sure that ReturnValue is a pointer operand of store instruction. Regression test is also added.

This fixes PR24386.

[kuhar]

ping

[aadg]

Just a minor nitpick (see below)

lib/CodeGen/CGCall.cpp

2332 ↗

(On Diff #33304)

It might be worth stating in a comment why this is needed --- for the benefit of people who will read this code later.

[aadg]

Yet another comment (see below).

lib/CodeGen/CGCall.cpp

2332 ↗

(On Diff #33304)

I also note that those lines are looking very much like those in the if above (handling the multiple use of the return value case)... This issue was fixed above, but not here. Would it be possible to avoid code duplication ?

[kuhar]

Some refactoring + comments added.

[aadg]

This looks ok to me.

[kuhar]

Merge with the latest changes to CGCall.cpp

cfe/trunk/lib/CodeGen/CGCall.cpp

if (CGF.shouldUseFusedARCCalls())
     if (llvm::Value *fused = tryEmitFusedAutoreleaseOfResult(CGF, result))
       return fused;
 
   return CGF.EmitARCAutoreleaseReturnValue(result);
 }
 
 /// Heuristically search for a dominating store to the return-value slot.
 static llvm::StoreInst *findDominatingStoreToReturnValue(CodeGenFunction &CGF) {
+  // Check if a User is a store which pointerOperand is the ReturnValue.
+  // We are looking for stores to the ReturnValue, not for stores of the
+  // ReturnValue to some other location.
+  auto GetStoreIfValid = [&CGF](llvm::User *U) -> llvm::StoreInst * {
+    auto *SI = dyn_cast<llvm::StoreInst>(U);
+    if (!SI || SI->getPointerOperand() != CGF.ReturnValue.getPointer())
+      return nullptr;
+    // These aren't actually possible for non-coerced returns, and we
+    // only care about non-coerced returns on this code path.
+    assert(!SI->isAtomic() && !SI->isVolatile());
+    return SI;
+  };
   // If there are multiple uses of the return-value slot, just check
   // for something immediately preceding the IP.  Sometimes this can
   // happen with how we generate implicit-returns; it can also happen
   // with noreturn cleanups.
   if (!CGF.ReturnValue.getPointer()->hasOneUse()) {
     llvm::BasicBlock *IP = CGF.Builder.GetInsertBlock();
     if (IP->empty()) return nullptr;
     llvm::Instruction *I = &IP->back();
for (llvm::BasicBlock::reverse_iterator II = IP->rbegin(),
           if (isa<llvm::BitCastInst>(&*II) && (CastAddr == &*II))
             continue;
         }
       }
       I = &*II;
       break;
     }
 
-    llvm::StoreInst *store = dyn_cast<llvm::StoreInst>(I);
-    if (!store) return nullptr;
-    if (store->getPointerOperand() != CGF.ReturnValue.getPointer())
-      return nullptr;
-    assert(!store->isAtomic() && !store->isVolatile()); // see below
-    return store;
+    return GetStoreIfValid(I);
   }
 
   llvm::StoreInst *store =
-    dyn_cast<llvm::StoreInst>(CGF.ReturnValue.getPointer()->user_back());
+      GetStoreIfValid(CGF.ReturnValue.getPointer()->user_back());
   if (!store) return nullptr;
 
-  // These aren't actually possible for non-coerced returns, and we
-  // only care about non-coerced returns on this code path.
-  assert(!store->isAtomic() && !store->isVolatile());
-
   // Now do a first-and-dirty dominance check: just walk up the
   // single-predecessors chain from the current insertion point.
   llvm::BasicBlock *StoreBB = store->getParent();
   llvm::BasicBlock *IP = CGF.Builder.GetInsertBlock();
   while (IP != StoreBB) {
     if (!(IP = IP->getSinglePredecessor()))
       return nullptr;
   }

cfe/trunk/test/CodeGen/arm_function_epilog.cpp

+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -triple armv7-none-linux-androideabi -target-abi aapcs-linux -mfloat-abi hard -x c++ -emit-llvm %s -o - | FileCheck %s
+
+struct Vec2 {
+    union { struct { float x, y; };
+            float data[2];
+    };
+};
+
+// CHECK: define arm_aapcs_vfpcc %struct.Vec2 @_Z7getVec2v()
+// CHECK: ret %struct.Vec2
+Vec2 getVec2() {
+    Vec2 out;
+    union { Vec2* v; unsigned char* u; } x;
+    x.v = &out;
+    return out;
+}