[PATCH] D72547: [llvm] Make new pass manager's OptimizationLevel a class

[Teresa Johnson via Phabricator via llvm-commits]

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