[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Minor nits around redundant predicates for SROA. With thouse fixed, LGTM.

I'd really love to find a way to make TCO debuggable so that we don't lose that. I'm particularly worried about code that relies on it to not run out of stack. Not sure what the best thing to do here is though. Anyways, not relevant for this iteration. I mostly feel bad for a potential future re-churn of all the tests. ;]



================
Comment at: llvm/lib/Passes/PassBuilder.cpp:395
   // scalars.
-  FPM.addPass(SROA());
+  if (Level >= O1)
+    FPM.addPass(SROA());
----------------
We know `O0` isn't used here, so this should be a no-op.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:263
   FPM.add(createCFGSimplificationPass());
-  FPM.add(createSROAPass());
+  if (OptLevel >= 1)
+    FPM.add(createSROAPass());
----------------
We early exit at `O0` above, so this is a no-op.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:294
     MPM.add(createFunctionInliningPass(IP));
-    MPM.add(createSROAPass());
+    if (OptLevel >= 1)
+      MPM.add(createSROAPass());
----------------
We only reach here if `OptLevel > 0` so this should be redundant?


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:325
   // Break up aggregate allocas, using SSAUpdater.
-  MPM.add(createSROAPass());
+  if (OptLevel >= 1)
+    MPM.add(createSROAPass());
----------------
This doesn't have the assert, but I believe this is only used above `O0` as well. Maybe just add the assert?


================
Comment at: llvm/test/Other/new-pm-defaults.ll:253
+; CHECK-Os-NEXT: Finished llvm::Function pass manager run.
+; CHECK-Oz-NEXT: Finished llvm::Function pass manager run.
 ; CHECK-EP-SCALAR-LATE-NEXT: Running pass: NoOpFunctionPass
----------------
mehdi_amini wrote:
> Just a drive-by idea: you could have simplified the changes here with a new prefix: "CHECK-O23sz" that you could add to the non-O1 invocations.
> 
Good idea!


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

https://reviews.llvm.org/D65410