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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Sorry for delay sending these commetns -- i had them typed up and didn't mash send. =[[[

Also, to address (briefly) what Justin mentioned: I think getting cache locality of single visit of instructions is likely to be more valuable than the overhead of the indirect calls. Could be wrong, but we can also look to see if there is a problem.



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1045-1046
+class AttributeInferer {
+  // We provide a generic facility to infer many attributes at once
+  // customizing individual attributes handling by a handful of predicates.
+  using SkipFunctionPredicate = std::function<bool(const Function &)>;
----------------
This should be a (more expansive I suspect) doxygen comment on the type.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1047-1052
+  using SkipFunctionPredicate = std::function<bool(const Function &)>;
+  using InvalidInstructionPredicate = std::function<bool(Instruction &)>;
+  using SetAttributeFunc = std::function<void(Function &)>;
+
+  SmallVector<std::tuple<bool, SkipFunctionPredicate,
+                         InvalidInstructionPredicate, SetAttributeFunc>,
----------------
The tuple here should almost certainly be a little struct to simplify the code. At that point, I don't know that you need the type aliases.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1057-1063
+  /// setup to infer attribute that might be subject to derefinement
+  void inferDerefinableAttr(SkipFunctionPredicate SkipFunc,
+                            InvalidInstructionPredicate InvalidInstr,
+                            SetAttributeFunc SetAttr) {
+    AttributePredicates.push_back(
+        std::make_tuple(true, SkipFunc, InvalidInstr, SetAttr));
+  }
----------------
I think this should be named more along the lines of registering something as it doesn't actually do inference here.

Currently, the doxygen comment doesn't add much value. Instead, I would suggest that the doxygen comment should explain what the semantics of these predicates are, how they are used, and mention what is different about the derefinement.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1078
+/// attribute predicates stored before.
+bool AttributeInferer::run(const SCCNodeSet &SCCNodes) const {
+  unsigned AttrCount = AttributePredicates.size();
----------------
This routine is only going to be run once over an SCC. I think that suggests a much simpler implementation.

As we process the SCC nodes and the instructions within each, we can use a `remove_if` (or better `erase_if`) pattern so that as predicates fail, we simple remove the struct w/ the callbacks for that attribute.

Then at the end, we run all the callbacks that remain.

This should allow you to not have index based walks or the bit vectors.


Repository:
  rL LLVM

https://reviews.llvm.org/D44415