[PATCH] D15556: [RS4GC] Fix crash in the case that a live variable has a constant base.

[Philip Reames via llvm-commits]

reames added a comment.

Comments inline.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1620
@@ -1619,1 +1619,3 @@
   for (Value *L : LiveSet) {
+    Value *Base = PointerToBase.find(L)->second;
+    if (isa<Constant>(Base))
----------------
I think the general approach of filtering the liveset here can work, but this is the wrong place for it.  In particular, it's after the vector splitting and re-materialization logic.  The vector splitting probably isn't an issue - I'm about to revise/remove that anyways - but the remat logic definitely has coupling with the relocation logic.  

I think you probably should pull this out as a separate step between the recomputation of liveness after base pointer rewriting and the first use of the new liveness in splitVectorValues.  

I'll also require much better comments.  :)  For instance, why does simply rewriting the live value as a constant-expr fail?  (i.e. why is the right approach?)

p.s. A possible alternate approach would be to stop filtering constants from the liveset.  In that case, we would have the live constant base in the liveset.  Have you considered this option?

================
Comment at: test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll:10
@@ +9,3 @@
+  %derived2 = getelementptr i8, i8 addrspace(1)* @global, i64 2
+  %select = select i1 undef, i8 addrspace(1)* %derived1, i8 addrspace(1)* %derived2
+  %safepoint_token = call i32 (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 2882400000, i32 0, void ()* @extern, i32 0, i32 0, i32 0, i32 0)
----------------
instead of undef, use an i1 argument.  

A valid canonicalizing transform (i.e. simplifyInstruction) would break this test in a non-obvious way.

================
Comment at: test/Transforms/RewriteStatepointsForGC/base-pointers-12.ll:13
@@ +12,3 @@
+; CHECK-NOT: relocate
+  %load = load i8, i8 addrspace(1)* %select
+  ret i8 %load
----------------
Add a check for the load from select.


http://reviews.llvm.org/D15556