Page MenuHomePhabricator

[VPlan] Support to widen select intructions in VPlan native path
AcceptedPublic

Authored by Kazhuu on Sat, Feb 20, 11:21 PM.

Details

Summary

Add support to widen select instructions in VPlan native path by using a correct recipe when such instructions are encountered. This is already used by inner loop vectorizer.

Previously select instructions get handled by the wrong recipe and resulted in unreachable instruction errors like this one: https://bugs.llvm.org/show_bug.cgi?id=48139.

Diff Detail

Event Timeline

Kazhuu created this revision.Sat, Feb 20, 11:21 PM
Kazhuu requested review of this revision.Sat, Feb 20, 11:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Feb 20, 11:21 PM
Kazhuu edited the summary of this revision. (Show Details)Sat, Feb 20, 11:27 PM
Kazhuu edited the summary of this revision. (Show Details)Sat, Feb 20, 11:35 PM
Kazhuu added reviewers: fhahn, rengolin, gilr.
Kazhuu updated this revision to Diff 325300.Sun, Feb 21, 7:02 AM

Fix failing unit tests.

fhahn requested changes to this revision.Mon, Feb 22, 1:10 PM

Thanks for the patch! I think this can be split into 2 indepdennt patches (one for selects, one for calls). Also, please add new minimal test cases for both the call & select cases.

This revision now requires changes to proceed.Mon, Feb 22, 1:10 PM
Kazhuu updated this revision to Diff 325716.Tue, Feb 23, 2:02 AM
Kazhuu retitled this revision from [VPlan] Support to widen call and select intructions in VPlan native path to [VPlan] Support to widen select intructions in VPlan native path.
Kazhuu edited the summary of this revision. (Show Details)

Rebase with latest changes, add support for select instruction widening only in this patch and add test case for it. Add call instruction widening as a separate patch later.

Call instruction widening as a separate patch here: https://reviews.llvm.org/D97278

Kazhuu edited the summary of this revision. (Show Details)Tue, Feb 23, 6:10 AM
fhahn added inline comments.Tue, Feb 23, 8:06 AM
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
20

We only need SE, can you pass that directly?

75

I think we should test all possible for loop invariant conditions (in inner & outer loop).

Kazhuu added inline comments.Tue, Feb 23, 9:25 AM
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
75

Can you elaborate a bit what you mean? Like test that is the condition invariant for all inner and outer loops?

Kazhuu updated this revision to Diff 325997.Tue, Feb 23, 11:50 PM

Removed use of PredicatedScalarEvolution and use ScalaEvolution instead to clean up diff.

Kazhuu marked an inline comment as done.Tue, Feb 23, 11:53 PM
Kazhuu added inline comments.
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
20

Thanks for the comment! I didn't pay attention enough for this. Removing PSE also cleaned up code in tests as well.

fhahn added inline comments.Wed, Feb 24, 7:18 AM
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
75

Currently the condition of the select in the inner loop is invariant (because it's a function argument). We should have tests with conditions that are invariant only in parts of the loop (e.g. a select with a condition that depends on inner & outer IVs in the inner loop, a select with a condition that depends on the outer IV in the inner loop).

Kazhuu updated this revision to Diff 326384.Thu, Feb 25, 7:29 AM
Kazhuu marked an inline comment as done.

Add test cases for different select conditions and clean up test to only check relevant widened select lines. I checked other tests that other people have written and some of them also check for the relevant lines only, not all lines generated by the python script. So I did the same. Any opinions is this a preferred way?

fhahn accepted this revision.Tue, Mar 2, 1:39 AM

LGTM with a few additional changes.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
20

I think it is more common to pass generic analysis at the end in LLVM. Might be good to adjust.

llvm/test/Transforms/LoopVectorize/vplan-widen-select-instruction.ll
1

Looks like the tests are not auto-generated. Probably best to drop this?

28

the body could probably be a bit further simplified by getting rid of the reduction in the inner loop and instead storing the select result directly in the inner loop (instead of storing sum in the outer loop latch). Same for the other tests.

With the reduced loops, could you add checks ensuring the the select is also in the expected blocks and the operands and result of the select are the values we expect/used as expected?

This revision is now accepted and ready to land.Tue, Mar 2, 1:39 AM