[PATCH] D24000: [experimental] Add support for live-in semantics of values in deopt bundles

[Philip Reames via llvm-commits]

reames added a comment.

Updated patch to follow shortly.


================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:455
@@ -441,2 +454,3 @@
     }
+    assert(SI.Bases.size() == SI.Ptrs.size() && "pointer without base?");
   } else {
----------------
sanjoy wrote:
> Is this new assert related to this change?  (It's fine to land with this change or separately, I'm just curious)
> 
> Also the convention for assertion failure messages in the rest of LLVM seems to be `"Pointer without base(?|!)"`, so we should not deviate from that unless we need to.
It's new.  It should already hold, but I noticed it while working on this.  

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:473
@@ +472,3 @@
+  const bool LiveInDeopt = UseLiveInDeopt &&
+    hasBitSet(SI.StatepointFlags, StatepointFlags::DeoptLiveIn);
+
----------------
sanjoy wrote:
> Not sure if `hasBitSet` is making things clearer.  Does `UseLiveInDeopt && (SI.StatepointFlags & StatepointFlags::DeoptLiveIn)` compile?
Good catch.  This was from I originally was trying to support live-out in the same patch and needed two bits to encode the options.  Simplified.  (In retrospect, I'd also picked a terrible name for the original purpose.)

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:476
@@ +475,3 @@
+  auto isGCValue =[&](const Value *V) {
+    return is_contained(SI.Ptrs, V) || is_contained(SI.Bases, V);
+  };
----------------
sanjoy wrote:
> Can this be
> 
> ```
> assert(is_contained(SI.Bases, V) implies is_contained(SI.Ptrs, V));
> return is_contained(SI.Ptrs, V);
> ```
> 
> ?
No it can't.  You can have a single use of the derived pointer after the call.  This would produce a statepoint with a single gc.relocate with the pair (base, derived), but not (base, base).  This is correct code generation for this case.  


https://reviews.llvm.org/D24000