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

Tue Nov 29 18:38:45 PST 2022

shchenz 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());
----------------
lkail wrote:
> 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`.
https://reviews.llvm.org/D138973 is created, so any pass that is wrongly added to O0-pipeline should be detected by O0 pipeline test now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138265