[RewriteStatepointsForGC] Fix a relocation bug w.r.t values defined by invoke instructions

[• chenli]

Summary

RewriteStatepointsForGC pass emits an alloca for each GC pointer which will be relocated. It then inserts stores after def and all relocations, and inserts loads before each use as well. In the end, mem2reg is used to update IR with relocations in SSA form.

However, there is a problem with inserting stores for values defined by invoke instructions. The code didn't expect a def was a terminator instruction, and inserting instructions after these terminators resulted in malformed IR.

This patch fixes this problem by handling invoke instructions as a special case. If the def is an invoke instruction, the store will be inserted at the beginning of the normal destination block. Since return value from invoke instruction does not dominate the unwind destination block, no action is needed there.

[sanjoy]

Comments inline.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1570 ↗

(On Diff #20805)

Minor nit: currently the abort string will be "The only ... a value is < several spaces > InvokeInst which ...". I think you can just split the string literal here:

assert(!cast<Instruction>(def)->isTerminator() &&
       "The only TerminatorInst that can produce a value is "
       "InvokeInst which is handled above.");

1572 ↗

(On Diff #20805)

You have an isa<Instruction>(def) followed by two cast<Instruction>(def)s. It might just be better to dyn_cast to an Instruction in the condition like you've done for the InvokeInst.

[• chenli]

Update patch w.r.t Sanjoy's comments.

[reames]

test/Transforms/RewriteStatepointsForGC/relocate_invoke_result.ll

2 ↗

(On Diff #21049)

Please use FileCheck to check the output of this test.

[• chenli]

Update test case with FileCheck w.r.t Philip's comments.

[reames]

LGTM. Do you have commit access or should I submit for you?

[• chenli]

Hi Philip,
I don't have commit access. Please help me submit it. Thanks!

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

1572 ↗

(On Diff #20805)

Yes, I will clean up this part.

llvm/trunk/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

for (Instruction *use : uses) {
         use->replaceUsesOfWith(def, load);
       }
     }
 
     // emit store for the initial gc value
     // store must be inserted after load, otherwise store will be in alloca's
     // use list and an extra load will be inserted before it
     StoreInst *store = new StoreInst(def, alloca);
-    if (isa<Instruction>(def)) {
-      store->insertAfter(cast<Instruction>(def));
+    if (Instruction *inst = dyn_cast<Instruction>(def)) {
+      if (InvokeInst *invoke = dyn_cast<InvokeInst>(inst)) {
+        // InvokeInst is a TerminatorInst so the store need to be inserted
+        // into its normal destination block.
+        BasicBlock *normalDest = invoke->getNormalDest();
+        store->insertBefore(normalDest->getFirstNonPHI());
+      } else {
+        assert(!inst->isTerminator() &&
+               "The only TerminatorInst that can produce a value is "
+               "InvokeInst which is handled above.");
+         store->insertAfter(inst);
+      }
     } else {
       assert((isa<Argument>(def) || isa<GlobalVariable>(def) ||
               (isa<Constant>(def) && cast<Constant>(def)->isNullValue())) &&
              "Must be argument or global");
       store->insertAfter(cast<Instruction>(alloca));
     }
   }
 

llvm/trunk/test/Transforms/RewriteStatepointsForGC/relocate_invoke_result.ll

+;; RUN: opt -rewrite-statepoints-for-gc -verify -S < %s | FileCheck %s
+
+;; This test is to verify that RewriteStatepointsForGC correctly relocates values
+;; defined by invoke instruction results. 
+
+declare i64* addrspace(1)* @non_gc_call()
+
+declare void @gc_call()
+
+declare i32* @fake_personality_function()
+
+; Function Attrs: nounwind
+define i64* addrspace(1)* @test() gc "statepoint-example" {
+entry:
+  %obj = invoke i64* addrspace(1)* @non_gc_call()
+          to label %normal_dest unwind label %unwind_dest
+
+unwind_dest: 
+  %lpad = landingpad { i8*, i32 } personality i32* ()* @fake_personality_function
+          cleanup
+  resume { i8*, i32 } undef
+
+normal_dest:
+;; CHECK-LABEL: normal_dest:
+;; CHECK-NEXT: gc.statepoint
+;; CHECK-NEXT: %obj.relocated = call coldcc i64* addrspace(1)*                                      
+  %safepoint_token = call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* @gc_call, i32 0, i32 0, i32 5, i32 0, i32 -1, i32 0, i32 0, i32 0)
+  ret i64* addrspace(1)* %obj
+}
+
+declare i32 @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()*, i32, i32, ...)
+
+