This is an archive of the discontinued LLVM Phabricator instance.

[NPM] Port -loop-versioning-licm to NPM
ClosedPublic

Authored by TaWeiTu on Oct 14 2020, 1:39 AM.

Diff Detail

Unit TestsFailed

Event Timeline

TaWeiTu created this revision.Oct 14 2020, 1:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 1:39 AM
TaWeiTu requested review of this revision.Oct 14 2020, 1:39 AM
TaWeiTu updated this revision to Diff 298071.Oct 14 2020, 1:45 AM
TaWeiTu updated this revision to Diff 298076.Oct 14 2020, 2:05 AM

Thanks! Can you add a second RUN line with -passes=loop-versioning-licm to an existing test?

aeubanks added inline comments.Oct 14 2020, 9:18 AM
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.

TaWeiTu updated this revision to Diff 298179.Oct 14 2020, 10:34 AM

Address comments.

Thanks! Can you add a second RUN line with -passes=loop-versioning-licm to an existing test?

Thanks for the review! I will update the tests later.

aeubanks added inline comments.Oct 14 2020, 10:39 AM
llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp
147

I meant can we can treat LoopAccessInfo just like the other analyses like DominatorTree?
Creating it inside LoopVersioningLICMPass::run/LoopVersioningLICMLegacyPass::runOnLoop rather than passing a function_ref. So GetLAI is unnecessary.

TaWeiTu added inline comments.Oct 14 2020, 10:55 AM
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.
What's your opinion on this? Thanks!

aeubanks added inline comments.Oct 14 2020, 11:00 AM
llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp
147

Ah that makes sense, sgtm. A comment would be good.

TaWeiTu updated this revision to Diff 298391.Oct 15 2020, 8:18 AM

Add comments regarding GetLAI. Port tests to NPM.

TaWeiTu added inline comments.Oct 15 2020, 8:21 AM
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.

TaWeiTu added a comment.EditedOct 15 2020, 10:04 AM

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.

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.

./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.

TaWeiTu updated this revision to Diff 298553.Oct 15 2020, 11:57 PM

Fix tests.

TaWeiTu updated this revision to Diff 298554.Oct 16 2020, 12:00 AM
TaWeiTu updated this revision to Diff 298572.Oct 16 2020, 2:45 AM
TaWeiTu added inline comments.Oct 16 2020, 2:56 AM
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.
Is there a better way to do so?

aeubanks added inline comments.Oct 16 2020, 9:21 AM
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?

TaWeiTu added inline comments.Oct 16 2020, 9:36 AM
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.
I'm not sure whether the simplified-form should be preserved. The loop terminology page says that LCSSA should be preserved, but I couldn't find similar discussion about loop-simplify.
However, since the versioning part of loop-versioning-licm is done via loop-versioning, I suspect that the latter preserves simplified form.
Should I add dummy exit blocks after the versioned loops to make it simplified?

I'm not sure, maybe asbirlea has better comments.
Otherwise lgtm

TaWeiTu updated this revision to Diff 298680.Oct 16 2020, 10:58 AM

OK! Thanks for your review. I've submitted a patch D89569 which adds dedicated exits to the versioned loops.

aeubanks accepted this revision.Oct 22 2020, 9:13 AM

lgtm, but https://reviews.llvm.org/D89569 should be submitted first

This revision is now accepted and ready to land.Oct 22 2020, 9:13 AM

lgtm, but https://reviews.llvm.org/D89569 should be submitted first

Of course. Thanks!

This revision was landed with ongoing or failed builds.Oct 24 2020, 6:51 AM
This revision was automatically updated to reflect the committed changes.