[PATCH] D127453: [NFC] Implement non-trivial LoopUnswitch with FunctionPass, and integrate it into O3 pipeline

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

we should definitely not completely copy the entire file, please keep the pass in the same file and reuse as much code as possible (which should be most of it I'd imagine)

the summary shouldn't be the title copied, the summary should explain the motivation behind this (make sure it's properly updated both locally in the commit message and also in phabricator, arc/phab isn't great about keeping them in sync)

the title is a bit confusing, it says it's NFC but also says it's changing the -O3 pipeline. I'd remove the NFC since this is actually adding functionality, and say "[SimpleLoopUnswitch] Add flag to...". But actually even better would be to move the function pass implementation into its own patch and have its addition to the pipeline be a different patch

there should also be tests for the new pass, probably reusing the existing ones is good enough



================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:172
+static cl::opt<bool> UseFuncNonTrivialUnswitching(
+    "use-FuncPass-nontrivial-unswitch", cl::init(false), cl::Hidden,
+    cl::ZeroOrMore,
----------------
other nearby flag names generally don't use capital letters, do the same here. I'd say something like `enable-function-pass-nontrivial-unswitch`


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:496
                         /*AllowSpeculation=*/true));
-  LPM1.addPass(
-      SimpleLoopUnswitchPass(/* NonTrivial */ Level == OptimizationLevel::O3 &&
-                             EnableO3NonTrivialUnswitching));
-  if (EnableLoopFlatten)
-    LPM1.addPass(LoopFlattenPass());
+  bool requireFuncNonTrivialUnswitching = (Level == OptimizationLevel::O3) &&
+                                          EnableO3NonTrivialUnswitching &&
----------------
follow surrounding convention, in this case capitalization of local variables


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:504
+    if (EnableLoopFlatten)
+      LPM1.addPass(LoopFlattenPass());
+  } else {
----------------
loop flatten isn't enabled by default, let's not worry about keeping it exactly in the same order with unswitching, that would simplify this code a bit


================
Comment at: llvm/lib/Passes/PassRegistry.def:281
 FUNCTION_PASS("flattencfg", FlattenCFGPass())
+FUNCTION_PASS("func-simple-loop-unswitch", FuncSimpleLoopUnswitchPass())
 FUNCTION_PASS("make-guards-explicit", MakeGuardsExplicitPass())
----------------
I'd prefer `simple-loop-unswitch-func`

unrelated, we should really rename this to just `loop-unswitch` now that we've deleted the old loop unswitch pass


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127453