Details
- Reviewers
asbirlea ychen aeubanks - Commits
- rG65a36bbc3d79: [NPM] Port -loop-versioning-licm to NPM
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks! Can you add a second RUN line with -passes=loop-versioning-licm to an existing test?
llvm/include/llvm/Transforms/Scalar/LoopVersioningLICM.h | ||
---|---|---|
18–19 | These shouldn't be necessary | |
llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp | ||
147 | is the LoopAccessLegacyAnalysis necessary? Can LoopVersioningLICM only need the LoopAccessInfo passed in from callers? Looks like both the legacy and NPM pass can create the proper LoopAccessInfo. |
llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp | ||
---|---|---|
147 | I meant can we can treat LoopAccessInfo just like the other analyses like DominatorTree? |
llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp | ||
---|---|---|
147 | The reason I used GetLAI in the first place is that we might hit an early return with instructions that are not safe for versioning and we don't need to analyze the whole loop in this case. |
llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp | ||
---|---|---|
147 | Ah that makes sense, sgtm. A comment would be good. |
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll | ||
---|---|---|
2 | I was not able to fix the remaining part of this test and loopversioningLICM2.ll yet, but it seemed to be more related to LICM itself. |
To test the NPM, don't add -enable-new-pm, but use the proper NPM flag -passes. So e.g. -passes=default<O1>,loop-versioning-licm,licm in place of -enable-new-pm -O1 -loop-versioning-licm -licm.
The failing tests seem fairly brittle and likely fail because the O1 pipeline is slightly different. Especially the names. Maybe the test should instead contain the output IR of running the existing test under -O1, then as the test itself run that IR through the proper passes.
OK. Will update the tests.
The failing tests seem fairly brittle and likely fail because the O1 pipeline is slightly different. Especially the names. Maybe the test should instead contain the output IR of running the existing test under -O1, then as the test itself run that IR through the proper passes.
From what I've seen the tests have already been passed through the O1 pipeline.
Since by specifying -O1 -loop-versioning-licm -licm the O1 pipeline runs after the two passes, even though NPM and LPM produce (almost) the same IR, the difference between the O1 pipelines that run afterward results in the failures.
So, one quick fix would be to run the IR through the two passes in NPM and then pipe it into the LPM O1 pipeline, but I find it quite inelegant.
What's your opinion? Thanks!
./build/rel/bin/opt -S -o - -enable-new-pm=0 -O1 -loop-versioning-licm -licm -debug-pass=Structure llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll --print-before-all shows that -O1 runs first, then the LICM passes. In opt.cpp it checks the order they are passed on the command line and adds them in that order (including -O#).
So I think running it through the tests through legacy -O1 and using that as the actual test would work.
Ah, my bad, thanks for clarifying! However,
./bin/opt -S -O1 loopversioningLICM1.ll -o O1.ll ./bin/opt -S -loop-versioning-licm -licm O1.ll
does not seem to be working for me for some unknown reasons. I'll dive into it more deeply tomorrow.
Thanks for your time.
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll | ||
---|---|---|
2 | Running the two passes separately here to ensure the simplified and LCSSA loop structured of the versioned loops so that LICM works properly. |
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll | ||
---|---|---|
2 | I think that's the proper way. I'm not super familiar with loop passes, I believe they must preserve LCSSA form, but do loop passes need to make sure that loop-simplify is a no-op if run after? |
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll | ||
---|---|---|
2 | This pass does preserve LCSSA judging from the assertion in LICM. The problem is that the LICM demands the loop to have a dedicated exit. |
OK! Thanks for your review. I've submitted a patch D89569 which adds dedicated exits to the versioned loops.
These shouldn't be necessary