CGCleanup: Use correct insertion point for AllocaInst

[GorNishanov]

Summary

This is a follow up for the rL297084 which made sure that dominance is preserved during expression cleanup emission.
We ran into a case where dominance fixup code was inserting the store in the middle of allocas.

%x = alloca i32, align 4
store i32* %x, i32** %tmp.exprcleanup, align 4 ; <===== HERE
%ref.tmp3 = alloca %struct.A, align 1
%agg.tmp5 = alloca %"struct.std::experimental::coroutines_v1::coroutine_handle.0", align 4
%tmp.exprcleanup = alloca i32*, align 4
%allocapt = bitcast i32 undef to i32
store i32 %0, i32* %.addr, align 4

This fix makes sure that if we are fixing up domination for an alloca instruction we will do it after the AllocaInsertPt

Posted an alternative fix here: https://reviews.llvm.org/D33733

[rnk]

lib/CodeGen/CGCleanup.cpp

464

This doesn't seem right, Inst could be a dynamic alloca. If it's static, we definitely don't need to store and reload it. All static allocas better be in the entry block... You might want to use isStaticAlloca, but that still feels like we're hacking around some deeper problem.

[GorNishanov]

Make sure that we use AllocaInsertPt for only static allocas in the entry block

[GorNishanov]

lib/CodeGen/CGCleanup.cpp

464

An extra context. Without the fix:

coro f(int) {
  int x = co_await A{}; // compiles fine
}

coro f(int) {
  int x = 42;
  x = co_await A{}; // does not compile due to broken IR
}

[GorNishanov]

Abandoning... Reid fixed the problem with "r304335 - Don't try to spill static allocas when emitting expr cleanups with branches"

Thank you Reid for looking into it!

lib/CodeGen/CGCleanup.cpp

for (unsigned I = 0, E = EHStack.getNumBranchFixups(); I != E; ++I) {
     // Add a case to the switch.
     Switch->addCase(Builder.getInt32(Fixup.DestinationIndex), Block);
   }
 
   if (ResolvedAny)
     EHStack.popNullFixups();
 }
 
+static bool isStaticAlloca(llvm::Value* V) {
+  if (auto *AI = dyn_cast<llvm::AllocaInst>(V))
+    return AI->isStaticAlloca();
+  return false;
+}
+
 /// Pops cleanup blocks until the given savepoint is reached.
 void CodeGenFunction::PopCleanupBlocks(
     EHScopeStack::stable_iterator Old,
     std::initializer_list<llvm::Value **> ValuesToReload) {
   assert(Old.isValid());
 
   bool HadBranches = false;
   while (EHStack.stable_begin() != Old) {
if (!Inst)
       continue;
     Address Tmp =
         CreateDefaultAlignTempAlloca(Inst->getType(), "tmp.exprcleanup");
 
     // Find an insertion point after Inst and spill it to the temporary.
     llvm::BasicBlock::iterator InsertBefore;
     if (auto *Invoke = dyn_cast<llvm::InvokeInst>(Inst))
       InsertBefore = Invoke->getNormalDest()->getFirstInsertionPt();
+    else if (isStaticAlloca(Inst))
+      InsertBefore = std::next(AllocaInsertPt->getIterator());
     else
       InsertBefore = std::next(Inst->getIterator());
     CGBuilderTy(CGM, &*InsertBefore).CreateStore(Inst, Tmp);
 
     // Reload the value at the current insertion point.
     *ReloadedValue = Builder.CreateLoad(Tmp);
   }
 }

test/CodeGenCoroutines/coro-await-domination.cpp

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 -emit-llvm %s -o - | FileCheck %s
+#include "Inputs/coroutine.h"
+
+using namespace std::experimental;
+
+struct coro {
+  struct promise_type {
+    coro get_return_object();
+    suspend_never initial_suspend();
+    suspend_never final_suspend();
+    void return_void();
+    static void unhandled_exception();
+  };
+};
+
+struct A {
+  ~A();
+  bool await_ready();
+  int await_resume();
+  template <typename F> void await_suspend(F);
+};
+
+// Verifies that domination is properly built during cleanup.
+// Without CGCleanup.cpp fix verifier was reporting:
+// Instruction does not dominate all uses!
+//  %tmp.exprcleanup = alloca i32*, align 8
+//  store i32* %x, i32** %tmp.exprcleanup, align 8
+
+
+// CHECK-LABEL: f(
+extern "C" coro f(int) {
+  int x = 42;
+  x = co_await A{};
+}