This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] move ctrloop pass before tail duplication
ClosedPublic

Authored by shchenz on Nov 17 2022, 11:22 PM.

Details

Reviewers
lkail
Group Reviewers
Restricted Project
Commits
rGb61ff0ca76a5: [PowerPC] move ctrloop pass before tail duplication
Summary

Tail duplication may modify the loop to a "non-canonical" form that CTR Loop pass can not
recognize. We fixed one issue in D135846. And we found in some other case, the loop is
changed to irreducible form. It is hard to fix this case in CTR loop pass, instead we reorder the
CTR loop pass before tail duplication pass and just after finalize-isel pass to avoid any unexpected
change to the loop form.

This patch:
1: reverted the change in d135846
2: move the ctrloop pass before tail duplication

Diff Detail

Unit TestsFailed

Event Timeline

shchenz created this revision.Nov 17 2022, 11:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 11:22 PM
shchenz requested review of this revision.Nov 17 2022, 11:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 11:22 PM
lkail added inline comments.Nov 23 2022, 11:16 PM
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
505

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.

shchenz added inline comments.Nov 24 2022, 2:04 AM
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
505

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?

lkail added inline comments.Nov 24 2022, 11:30 PM
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
505

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.

shchenz added inline comments.Nov 29 2022, 6:38 PM
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
505

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.

lkail accepted this revision as: lkail.Nov 30 2022, 6:29 PM

LGTM. Though there might be some pseudo instruction expansion changed the CFG, they are not breaking assumptions PPCCTRLoop made.

This revision is now accepted and ready to land.Nov 30 2022, 6:29 PM
This revision was landed with ongoing or failed builds.Dec 1 2022, 9:34 PM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/PowerPC/O3-pipeline.ll