This is an archive of the discontinued LLVM Phabricator instance.

[Passes] Remove legacy LoopUnswitch pass.
ClosedPublic

Authored by fhahn on Apr 25 2022, 3:49 AM.

Details

Summary

The legacy LoopUnswitch pass is only used in the legacy pass manager
pipeline, which is deprecated.

The NewPM replacement is SimpleLoopUnswitch and I think it is time to
remove the legacy LoopUnswitch code.

Fixes #31000.

Diff Detail

Event Timeline

fhahn created this revision.Apr 25 2022, 3:49 AM
Herald added a reviewer: bollu. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
fhahn requested review of this revision.Apr 25 2022, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 3:49 AM

After replacing the LoopUnswitch with SimpleLoopUnswitch in Polly's
CodeGenCleanup.cpp, there is one polly test failing. @Meinersbur is
updating the test the right way forward here?

From the FileCheck output, it looks like the change is just that the name of the basic block is different? polly debug output depends on LLVM IR names. Just updating the test should be fine.

aeubanks accepted this revision.Apr 25 2022, 9:05 AM
This revision is now accepted and ready to land.Apr 25 2022, 9:05 AM
Meinersbur requested changes to this revision.Apr 25 2022, 9:58 AM

Fix for Polly:

diff --git a/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp b/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
index 7aca971b868f..0030356f7990 100644
--- a/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
+++ b/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
@@ -397,7 +397,7 @@ void PassManagerBuilder::addFunctionSimplificationPasses(
   // TODO: Investigate promotion cap for O1.
   MPM.add(createLICMPass(LicmMssaOptCap, LicmMssaNoAccForPromotionCap,
                          /*AllowSpeculation=*/true));
-  MPM.add(createSimpleLoopUnswitchLegacyPass());
+  MPM.add(createSimpleLoopUnswitchLegacyPass(OptLevel == 3));
   // FIXME: We break the loop pass pipeline here in order to do full
   // simplifycfg. Eventually loop-simplifycfg should be enhanced to replace the
   // need for this.

This is what the NPM pipeline builder does (minus -enable-npm-O3-nontrivial-unswitch)

This revision now requires changes to proceed.Apr 25 2022, 9:58 AM

I've removed the failing opt-pipeline.ll test in 6f73bd781305266a747055875ce8352e5a36c809 since that's a legacy PM test

asbirlea accepted this revision.Apr 25 2022, 10:38 AM

Thank you for the cleanup!
Maybe worth a mention that this effectively enables SimpleLoopUnswitch for the legacy PM, in case any users need this clarification; more of a nit, considering the LPM is being removed.

fhahn edited the summary of this revision. (Show Details)Apr 26 2022, 7:32 AM
fhahn updated this revision to Diff 425210.Apr 26 2022, 7:34 AM
fhahn edited the summary of this revision. (Show Details)

i>>! In D124376#3472273, @Meinersbur wrote:

Fix for Polly:

Thanks, applied!

Thank you for the cleanup!
Maybe worth a mention that this effectively enables SimpleLoopUnswitch for the legacy PM, in case any users need this clarification; more of a nit, considering the LPM is being removed.

I'll updated the description, thanks!

fhahn edited the summary of this revision. (Show Details)Apr 28 2022, 5:58 AM

@Meinersbur It would be great if you could take another quick look and remove the change request :)

This revision is now accepted and ready to land.Apr 28 2022, 8:00 AM
This revision was landed with ongoing or failed builds.Apr 29 2022, 2:31 AM
This revision was automatically updated to reflect the committed changes.

Note Julia was still using the old PM with LoopUnswitch and when we switched to SimpleLoopUnswitch we found a case that significantly regressed https://discourse.llvm.org/t/newpm-loop-unswitch-regression-from-legacy-pm-to-new-pm/69823?u=vchuravy

llvm/test/Transforms/LoopUnswitch/simplify-with-nonvalness.ll