tejohnson added a comment.
I just have a few high level comments from looking through it just now. The
summary needs a fix since Os/Oz are in fact O2
<https://reviews.llvm.org/owners/package/2/> so OptLevel > 1 was not doing the
wrong thing.
================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:224
+
+ bool isOptimizingForSpeed() const { return Level > 0 && Level < 4; }
+ bool isOptimizingForSize() const { return Level == 4 || Level == 5; }
----------------
Can you add a comment as to why Os and Oz are considered as optimizing for
speed? I know this is for compatibility with the current code, but would be
good to document (and consider changing in the future).
================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:225
+ bool isOptimizingForSpeed() const { return Level > 0 && Level < 4; }
+ bool isOptimizingForSize() const { return Level == 4 || Level == 5; }
+ bool isO2Or3() const { return Level == 2 || Level == 3; }
----------------
This one is a little confusing to read, since at this point there is no
correlation between the values 4 and 5, and the Os and Oz static variables.
Consider making some constexpr values for each level, used in the methods here
and in the static variable initializations?
================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:226
+ bool isOptimizingForSize() const { return Level == 4 || Level == 5; }
+ bool isO2Or3() const { return Level == 2 || Level == 3; }
+ bool operator==(const OptimizationLevel &Other) const {
----------------
Since (as discussed off-patch), in the old PM Os and Oz are also opt level 2,
this should presumably return true for those as well. That should obviate the
need for many places in the patch where you are currently checking isO2Or3 ||
isOptimizingForSize, and you can just check isO2Or3.
================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:274
+ /// This is an interface that can be used to populate a \c
+ /// CGSCCAnalysisManager with all registered CGSCC analyses. Callers can
still
+ /// manually register any additional analyses. Callers can also pre-register
----------------
There are a lot of formatting changes throughout the patch that are unrelated
to your changes - it seems like you might have clang formatted the whole files?
Can you only include the changes related to the patch here, it's harder to
review with lots of spurious diffs.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72547/new/
https://reviews.llvm.org/D72547
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
