[PATCH] D44415: [PM][FunctionAttrs] add NoUnwind attribute inference to PostOrderFunctionAttrs pass
Justin Lebar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 16 05:38:43 PDT 2018
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
More information about the llvm-commits
mailing list