[PATCH] D138265: [PowerPC] move ctrloop pass before tail duplication

Thu Nov 24 23:30:28 PST 2022

lkail added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCTargetMachine.cpp:505
+  // canonical form of hardware loop from being destroied.
+  if (!DisableCTRLoops && getOptLevel() != CodeGenOpt::None)
+    addPass(createPPCCTRLoopsPass());
----------------
shchenz wrote:
> lkail wrote:
> > IIUC, `PPCCTRLoops` should be enabled iff `HardwareLoops` is enabled. We'd better put the switch-on logic into separate function, and query this function when adding PPCCTRLoops and HardwareLoopsPass(and maybe PPCCTRLoopsVerify together) to the pipeline.
> yes, you are right. `Hardware Loops` pass is needed for other two backend CTR loop passes. Currently they are all controlled by `DisableCTRLoops` flag which is defined in `PPCTargetMachine.cpp`. If we want to add the `getOptLevel()` as another guard that can be shared, we have to define a function in class `PPCPassConfig`. But I feel it is not making  sense to do so as `PPCPassConfig` currently should  only care about some "big" functionality, not a single pass...
> 
> To me, for all these three passes, querying the common `DisableCTRLoops` flag is enough. What do you think?
My thought is based on maintenance in the future. Another approach might be adding a `O0-pipeline.ll` test case to ensure consistency, i.e., `HardwareLoop` and `PPCCTRLoop` appear in `O3-pipeline.ll` simultaneously and none of them appear in `O0-pipeline`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138265