CodeGen: Fix missing debug loc due to alloca

[yaxunl]

Summary

Builder save/restores insertion pointer when emitting addr space cast
for alloca, but does not save/restore debug loc, which causes verifier
failure for certain call instructions.

This patch fixes that.

[rjmccall]

If this is something we generally need to be doing in all the places we temporarily save and restore the insertion point, we should fix the basic behavior of saveIP instead of adding explicit code to a bunch of separate places. Can we just override saveIP() on CGBuilder to return a struct that also includes the current debug location?

[yaxunl]

In D39069#903344, @rjmccall wrote:

If this is something we generally need to be doing in all the places we temporarily save and restore the insertion point, we should fix the basic behavior of saveIP instead of adding explicit code to a bunch of separate places. Can we just override saveIP() on CGBuilder to return a struct that also includes the current debug location?

IRBuilderBase::InsertPointGuard does that. Will use it instead.

[yaxunl]

Use InsertPointGuard and simplify test.

[rjmccall]

The code looks fine to me. The test seems very vague to me, but I'd like Dave to weigh in on that, because I'm not sure it's reasonable to test the exact pattern here.

Also, Dave, I don't know what the IR invariants around debug info are. Is this something we should be updating all of our uses of saveIP() to do if there's a possibility of emitting arbitrary code while the IP is saved?

[probinson]

Anytime the code between saveIP() and restoreIP() could set the current debug location, it needs to be saved/restored along with the insertion point. I have to say the problem is not obvious to me here, so maybe saveIP/restoreIP should be changed (or eliminated in favor of always using InsertPointGuard). I'm not seeing a lot of places where saveIP/restoreIP are used.

The test looks like all it's doing is verifying both calls have a debug location at all. It could verify that both calls have the _same_ debug location, which I would find much more robust and convincing.

[rjmccall]

In D39069#904507, @probinson wrote:

Anytime the code between saveIP() and restoreIP() could set the current debug location, it needs to be saved/restored along with the insertion point. I have to say the problem is not obvious to me here, so maybe saveIP/restoreIP should be changed (or eliminated in favor of always using InsertPointGuard). I'm not seeing a lot of places where saveIP/restoreIP are used.

Yeah, not too many. I'm not seeing any real reason not to just switch Clang over to always use InsertPointGuard. (Yaxun, you don't need to do this; I'm just making a note to myself.)

John.

[yaxunl]

Revised the test by Paul's comments.

[rjmccall]

Okay, LGTM.

cfe/trunk/lib/CodeGen/CGExpr.cpp

Address CodeGenFunction::CreateTempAlloca(llvm::Type *Ty, CharUnits Align,
   Alloca->setAlignment(Align.getQuantity());
   llvm::Value *V = Alloca;
   // Alloca always returns a pointer in alloca address space, which may
   // be different from the type defined by the language. For example,
   // in C++ the auto variables are in the default address space. Therefore
   // cast alloca to the default address space when necessary.
   if (CastToDefaultAddrSpace && getASTAllocaAddressSpace() != LangAS::Default) {
     auto DestAddrSpace = getContext().getTargetAddressSpace(LangAS::Default);
-    auto CurIP = Builder.saveIP();
+    llvm::IRBuilderBase::InsertPointGuard IPG(Builder);
     Builder.SetInsertPoint(AllocaInsertPt);
     V = getTargetHooks().performAddrSpaceCast(
         *this, V, getASTAllocaAddressSpace(), LangAS::Default,
         Ty->getPointerTo(DestAddrSpace), /*non-null*/ true);
-    Builder.restoreIP(CurIP);
   }
 
   return Address(V, Align);
 }
 
 /// CreateTempAlloca - This creates an alloca and inserts it into the entry
 /// block if \p ArraySize is nullptr, otherwise inserts it at the current
 /// insertion point of the builder.

cfe/trunk/test/CodeGenOpenCL/func-call-dbg-loc.cl

+// RUN: %clang_cc1 -triple amdgcn---amdgizcl -debug-info-kind=limited -O0 -emit-llvm -o - %s | FileCheck %s
+
+typedef struct
+{
+    int a;
+} Struct;
+
+Struct func1();
+
+void func2(Struct S);
+
+void func3()
+{
+    // CHECK: call i32 @func1() #{{[0-9]+}}, !dbg ![[LOC:[0-9]+]]
+    // CHECK: call void @func2(i32 %{{[0-9]+}}) #{{[0-9]+}}, !dbg ![[LOC]]
+    func2(func1());
+}
+