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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Overall I like this approach.



================
Comment at: llvm/lib/Passes/PassBuilder.cpp:246
 
-static bool isOptimizingForSize(PassBuilder::OptimizationLevel Level) {
-  switch (Level) {
-  case PassBuilder::O0:
-  case PassBuilder::O1:
-  case PassBuilder::O2:
-  case PassBuilder::O3:
-    return false;
-
-  case PassBuilder::Os:
-  case PassBuilder::Oz:
-    return true;
-  }
-  llvm_unreachable("Invalid optimization level!");
-}
+const PassBuilder::OptimizationLevel PassBuilder::OptimizationLevel::O0 = {0,
+                                                                           0};
----------------
Nit, it would be good to document the constant parameters, e.g. {/*SpeedLevel*/0, /*SizeLevel*/0}


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:407
   // Hoisting of scalars and load expressions.
-  if (Level > O1) {
+  if (Level.getSpeedupLevel() >= 2) {
     if (EnableGVNHoist)
----------------
Nit, all similar checks below are Level.getSpeedupLevel() > 1, make this one consistent.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:487
       PTO.LoopUnrolling)
-    LPM2.addPass(LoopFullUnrollPass(Level, /*OnlyWhenForced=*/false,
+    LPM2.addPass(LoopFullUnrollPass(Level.getSpeedupLevel(),
+                                    /*OnlyWhenForced=*/false,
----------------
This results in a behavior, change, right? I.e. we used to inadvertently get the O3 threshold for full unrolling with Oz/Os but no longer will. If so, make sure you note this in the patch summary. Also add a test.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:973
   if (EnableUnrollAndJam && PTO.LoopUnrolling) {
-    OptimizePM.addPass(LoopUnrollAndJamPass(Level));
+    OptimizePM.addPass(LoopUnrollAndJamPass(Level.getSpeedupLevel()));
   }
----------------
This one and the loop unrolling pass below will also get a change in behavior for Os/Oz, correct? That seems reasonable, but needs to be noted in summary and tested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72547/new/

https://reviews.llvm.org/D72547