[PATCH] D44415: [PM][FunctionAttrs] add NoUnwind attribute inference to PostOrderFunctionAttrs pass

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

I like it.

I have a suggestions on the comments, but that's...pretty normal for me.  :)

I think that if we don't go with the variadic templates thing -- which is probably a premature optimization, I trust Chandler here -- then going with Chandler's suggestion and getting rid of most or all of the bitsets would be a good simplification.



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1053
+  struct InferenceDescriptor {
+    /// Only "exact" definitions can be used to infer this attribute.
+    bool RequiresExactDefinition;
----------------
Can we expand upon what we mean by an "exact" definition?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1056
+
+    /// Returns true if this function does not have to be scanned.
+    std::function<bool(const Function &)> SkipFunction;
----------------
Should we say this function can be null (and if so, all functions need to be scanned)?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1066
+
+  SmallVector<InferenceDescriptor, 4> AttributePredicates;
+
----------------
Perhaps a different name for this vector, now that the struct has a name.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1100
+        continue;
+      // Check if this attribute ever needs scanning.
+      auto &AP = AttributePredicates[A];
----------------
This is kind of confusing right above the `auto &AP =` line, because it's not really describing that line...

Perhaps we could have a comment on the loop explaining at a higher level what we're doing?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1104
+      // This function is explicitly skipped from scan w/o breaking the main
+      // per-Function loop. Perhaps, it already has the attribute.
+      if (AP.SkipFunction(*F))
----------------
s/Perhaps,/Perhaps/


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1107
+        ScanAttrsHere[A] = false;
+      else {
+        if (F->isDeclaration() ||
----------------
```else if (...) {
}```

?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1110
+            (AP.RequiresExactDefinition && !F->hasExactDefinition())) {
+          // No instructions to scan, cant handle this attribute.
+          ScanAttrsHere[A] = false;
----------------
can't


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1129
+        if (AP.InstrBreaksAttribute(I)) {
+          // This instruction breaks attribute assumptions.
+          ScanAttrsHere[A] = false;
----------------
This comment is probably obvious from the line above now.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1155
+      auto &AP = AttributePredicates[A];
+      if (AP.SkipFunction(*F))
+        continue;
----------------
We should indicate somewhere (on the struct definition?) that if a function is skipped, it's skipped from both scanning *and* from having the attribute set on it.  That was surprising to me.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1165
+
+static bool InvalidInstrNonConvergent(Instruction &I,
+                                      const SCCNodeSet &SCCNodes) {
----------------
Perhaps instead of introducing this new "invalid" terminology, we should call it something related to one of the names in InferenceDescriptor?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1175
+                                    const SCCNodeSet &SCCNodes) {
+  bool InstMightUnwind = I.mayThrow();
+  if (InstMightUnwind) {
----------------
Perhaps this function would be clearer as:

```
if (I.mayThrow()) return false;
if (...) {
  if (SCCNodes.count(Callee)) {
     // I is a may-throw call to a function inside our SCC.  This doesn't invalidate our working assumption that the SCC is no-throw; we have to scan that other function.
    return false;
  }
}
// This instruction may throw, and it's not a call to another function in our SCC.  It therefore invalidates our non-throwing assumption.
return true;
```


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1206
+  AI.registerAttrInference(
+      {false /* RequiresExactDefinition */,
+       // Skip non-convergent functions.
----------------
Suggest `/*RequiresExactDefinition=*/false` this way it's not ambiguous whether we're saying that false means it does require an exact definition, or that it doesn't.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1221
+  // every callsite within the SCC is not throwing (except for calls to
+  // functions within the SCC). Note, that nounwind attribute suffers from
+  // derefinement - results may change depending on how functions are
----------------
s/Note, that/Note that/


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1236
+	 ++NumNoUnwind;
+       }});
+
----------------
Please re-clang-format


Repository:
  rL LLVM

https://reviews.llvm.org/D44415