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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

I don't have any deep problems with the patch, only some minor stylistic issues.

PS: This patch is one in a thousand. :)


================
Comment at: include/llvm/IR/Statepoint.h:37
@@ -36,1 +36,3 @@
                     ///< GC-aware code to code that is not GC-aware.
+  // Mark the deopt arguments associated with the statepoint as only being
+  // "live-in". By default, deopt arguments are "live-through".  "live-through"
----------------
Doxygen comments here.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:38
@@ +37,3 @@
+UseLiveInDeopt("experimental-live-in-deopt", cl::init(false), cl::Hidden,
+               cl::desc("Should deopt bundles be lowered as live in?"));
+
----------------
I've never seen a `cl::desc` phrased as a question, perhaps phrase it as "If true, lower deopt bundles as live-in"?

edit: After reading the code, I'm not sure if this is needed.  IIUC clients can already opt out of the new behavior by not specifying `"deopt-lowering"`, so I'm not sure we need another knob.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:455
@@ -441,2 +454,3 @@
     }
+    assert(SI.Bases.size() == SI.Ptrs.size() && "pointer without base?");
   } else {
----------------
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.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:463
@@ +462,3 @@
+  // Figure out what lowering strategy we're going to use for each part
+  // Note: Is is conservative correct to lower both "live-in" and "live-out"
+  // as "live-through". A "live-through" variable is one which is "live-in",
----------------
s/conservative/conservatively/

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:465
@@ +464,3 @@
+  // as "live-through". A "live-through" variable is one which is "live-in",
+  // "live-out", and live throughout the lifetime of the call (i.e. we can find
+  // it from any PC within the transative callee of the statepoint).  
----------------
Please add a small blurb on the difference between live-out and live-through.  As we discussed in person, this can be subtle.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:473
@@ +472,3 @@
+  const bool LiveInDeopt = UseLiveInDeopt &&
+    hasBitSet(SI.StatepointFlags, StatepointFlags::DeoptLiveIn);
+
----------------
Not sure if `hasBitSet` is making things clearer.  Does `UseLiveInDeopt && (SI.StatepointFlags & StatepointFlags::DeoptLiveIn)` compile?

================
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);
+  };
----------------
Can this be

```
assert(is_contained(SI.Bases, V) implies is_contained(SI.Ptrs, V));
return is_contained(SI.Ptrs, V);
```

?

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:514
@@ -480,3 +513,3 @@
     const Value *Base = SI.Bases[i];
-    lowerIncomingStatepointValue(Builder.getValue(Base), Ops, Builder);
+    lowerIncomingStatepointValue(Builder.getValue(Base), false, Ops, Builder);
 
----------------
I'd add a `/*LiveInOnly=*/` before the `false`, here and below.

================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:454
@@ +453,3 @@
+  case TargetOpcode::STATEPOINT: {
+    // For statepoints, fold deopt and gc arguements, but not call args.
+    StatepointOpers opers(&MI);
----------------
s/arguements/arguments/
s/args/arguments/

================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:455
@@ +454,3 @@
+    // For statepoints, fold deopt and gc arguements, but not call args.
+    StatepointOpers opers(&MI);
+    StartIdx = opers.getVarIdx();
----------------
s/opers/Opers/ 

or

`StartIdx = StatepointOpers(&MI).getVarIdx()`.

(I've fixed the existing violations).

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1277
@@ +1276,3 @@
+static StringRef getDeoptLowering(CallSite CS) {
+  static const char* DeoptLowering = "deopt-lowering";
+  if (CS.hasFnAttr(DeoptLowering)) {
----------------
Not sure if the `static` is adding much here.  I'd just stick with `const char *DeoptLowering = "deopt-lowering"`.  Also LLVM coding style associates the `*` with the variable name.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1340
@@ +1339,3 @@
+  else {
+    assert(DeoptLowering.equals("live-through") && "unsupported value");
+  }
----------------
Usually assertion failure messages are formatted as `"Unsupported value!"`.


https://reviews.llvm.org/D24000