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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added a comment.

I like the algorithm!  Just comments on comments, then I think I'm happy.  Dunno if @chandlerc wants to have a look or not...



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1054
+class AttributeInferer {
+
+public:
----------------
Remove newline?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1057
+  /// Describes a request for inference of a single attribute,
+  /// as well as tracks the state of inference.
+  struct InferenceDescriptor {
----------------
Update comment?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1076
+    /// If true, only "exact" definitions can be used to infer this attribute.
+    /// (See GlobalValue::isDefinitionExact)
+    bool RequiresExactDefinition;
----------------
s/)/.)/


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1103
+bool AttributeInferer::run(const SCCNodeSet &SCCNodes) {
+
+  SmallVector<InferenceDescriptor, 4> InferInSCC = InferenceDescriptors;
----------------
Remove newline?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1106
+  // Go through all the functions in SCC and check corresponding attribute
+  // assumptions for each of them.
   for (Function *F : SCCNodes) {
----------------
... "for each of them.  We'll remove attributes which aren't valid for the SCC from InferInSCC."


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1116
+      // This function is explicitly skipped from inference w/o breaking the
+      // main per-Function loop. Perhaps it already has the attribute.
+      if (ID.SkipFunction(*F))
----------------
This comment may be more confusing than not at this point.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1120
+
+      // Remove from further inference (invalidate) when meeting a function that
+      // has no instructions to scan/has unsuitable definition.
----------------
s/meeting/visiting/ (idom, sorry)


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1121
+      // Remove from further inference (invalidate) when meeting a function that
+      // has no instructions to scan/has unsuitable definition.
+      return (F->isDeclaration() ||
----------------
has an unsuitable definition


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1122
+      // has no instructions to scan/has unsuitable definition.
+      return (F->isDeclaration() ||
+              (ID.RequiresExactDefinition && !F->hasExactDefinition()));
----------------
Nit, remove outer parens?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1128
+    // All the attributes currently available for inference are good to go
+    // with this function unless explicitly skipped.
+    llvm::copy_if(
----------------
Suggest moving the comment up one line and saying something like

```For each attribute still in InferInSCC that doesn't skip F, check that the instructions in F are valid.```


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1138
+    for (Instruction &I : instructions(*F)) {
+
+      llvm::erase_if(InferInThisFunc, [&](const InferenceDescriptor &ID) {
----------------
Remove newline?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1142
+          return false;
+        // Remove attribute from inference.
+        llvm::erase_if(InferInSCC, [ID](const InferenceDescriptor &D) {
----------------
Perhaps this can get a "because" -- it's kind of the crux of the whole algorithm, and it's quite hidden here.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1151
+      if (InferInThisFunc.empty())
+        break;
     }
----------------
If you move this check to the top of the loop, then you don't need the check before the loop.


Repository:
  rL LLVM

https://reviews.llvm.org/D44415