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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1053
+  struct InferenceDescriptor {
+    /// Only "exact" definitions can be used to infer this attribute.
+    bool RequiresExactDefinition;
----------------
fedor.sergeev wrote:
> jlebar wrote:
> > Can we expand upon what we mean by an "exact" definition?
> I'm not sure how can I do that w/o duplicating a huge comment for GlobalValue::isDefinitionExact.
Could we just have a pointer to GlobalValue::isDefinitionExact?  As in

```If true, only "exact" definitions can be used to infer this attribute.  (See GlobalValue::isDefinitionExact.)```


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1236
+	 ++NumNoUnwind;
+       }});
+
----------------
fedor.sergeev wrote:
> jlebar wrote:
> > Please re-clang-format
> Err... clang-format behaves badly on this :(
> It formats two calls of registerAttrInference differently.
> So I took one format as is and hand-edited another invocation correspondingly.
> Will repeat the same before integration as well.
> So I took one format as is and hand-edited another invocation correspondingly.

That's fine with me, so long as we don't expect that future modifications to this part of the file will preserve the format.  People should be able to clang-format their changes and expect it's "good enough".


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1072
+    bool RequiresExactDefinition;
+
+    /// assumptions of this attribute still hold valid.
----------------
If we're going to mix both "properties" of the attribute and "state" of the algorithm in the same class, can we at least have a comment indicating that this is what we're doing, and all variables below this point are mutable state used by the algorithm?


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1073
+
+    /// assumptions of this attribute still hold valid.
+    bool Valid = true;
----------------
Capital


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1073
+
+    /// assumptions of this attribute still hold valid.
+    bool Valid = true;
----------------
jlebar wrote:
> Capital
s/still hold valid/are still valid/


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1080
+
+    /// true for "valid" attributes that went through at least one instruction
+    /// scan
----------------
Capital


================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1082
+    /// scan
+    bool SuccessfullyScanned = false;
+
----------------
I don't know how Chandler feels about it, but personally I'm not wild about the approach of sticking state into this struct.  It uses the descriptor class for two quite different purposes, and perhaps worse, it's really easy to have a bug where we forget to reset ScanCurrent at the top of the loop...

I *think* Chandler's suggestion was, instead of tracking the state with a bitset, track it by removing elements from a list (or adding them to a list of "attributes to use when scanning this function").

It looks like the difficulty you face here is that the list you want to remove elements from is also the list you're iterating over?  I'd be OK with pretty much any of the obvious solutions to that -- remove and adjust the loop index, use an std::list and remove in place, remove after the loop completes...

Dunno if Chandler has feels about this too.


Repository:
  rL LLVM

https://reviews.llvm.org/D44415