This is an archive of the discontinued LLVM Phabricator instance.

[SLP]No need to schedule/check parent for extract{element/value} instruction.
ClosedPublic

Authored by ABataev on Aug 25 2021, 8:03 AM.

Details

Summary

The instruction extractelement/extractvalue are not required to
be scheduled since they only depend on the source vector/aggregate (with
constant indices), smae applies to the parent basic block checks.
Improves compile time and saves scheduling budget.

Diff Detail

Event Timeline

ABataev created this revision.Aug 25 2021, 8:03 AM
ABataev requested review of this revision.Aug 25 2021, 8:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2021, 8:03 AM

couple of minors

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
209

Add description comment

211

clang-format has made this pretty messy - maybe split it up?

ABataev updated this revision to Diff 368668.Aug 25 2021, 9:24 AM

Rebase + address comments

RKSimon accepted this revision.Aug 25 2021, 9:27 AM

LGTM cheers

This revision is now accepted and ready to land.Aug 25 2021, 9:27 AM
This revision was landed with ongoing or failed builds.Aug 25 2021, 9:45 AM
This revision was automatically updated to reflect the committed changes.
lebedev.ri added inline comments.
llvm/test/Transforms/SLPVectorizer/X86/remark_extract_broadcast.ll
19–25

Passing-by remark: i suspect the extract/insert aren't needed here,
the splat/broadcast shuffle can just operate on the load?

ABataev added inline comments.Aug 25 2021, 9:54 AM
llvm/test/Transforms/SLPVectorizer/X86/remark_extract_broadcast.ll
19–25

Yes, I believe so and hope D107966 will fix it. Will check it later.

It looks like this patch causes an issue reported here: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152411.html. Reverting fixes it.

It looks like this patch causes an issue reported here: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152411.html. Reverting fixes it.

Are you able to see a quick fix? Otherwise we may need to revert and commit the test case for it to be addressed when @ABataev is back.

Are you able to see a quick fix? Otherwise we may need to revert and commit the test case for it to be addressed when @ABataev is back.

Yes, I'm looking for a quick fix. I've answered to thread that I'm to revert it if no fix in a short time.

Are you able to see a quick fix? Otherwise we may need to revert and commit the test case for it to be addressed when @ABataev is back.

Yes, I'm looking for a quick fix. I've answered to thread that I'm to revert it if no fix in a short time.

Looks like it crashes on shuffles, just need to add a check for shuffles in assert.

anton-afanasyev reopened this revision.Aug 31 2021, 5:12 AM

Looks like it crashes on shuffles, just need to add a check for shuffles in assert.

No, it crashes on extractelement with non-constant index. I've commented this inline.
The current crash could just be fixed by removing one assert check, but I've found several more minors, so may be it worth to revert? I think this should be recommited with negative tests for extractelement as wel as for insertelement (are we able to schedule insertelement with non-constant index now?)

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
221

Btw, this second ExtractValueInst is redundant, it's checked above.

234

Why do we need this bypass? Looks like this check isn't related to allSameBlock(). One can imagine vector-like-insts with const ops in different BBs. We can explicitly do this check outside allSameBlock() wherever need.

5959

all_of(VL, isVectorLikeInstWithConstOps) should be here instead?

6055

all_of(VL, isVectorLikeInstWithConstOps) should be here instead as well?

6095–6097

We assert here for bundle member not being vector-like-inst-with-const-ops, but check only S.OpValue before in tryScheduleBundle(). Remove this new check?

This revision is now accepted and ready to land.Aug 31 2021, 5:12 AM

Looks like it crashes on shuffles, just need to add a check for shuffles in assert.

No, it crashes on extractelement with non-constant index. I've commented this inline.
The current crash could just be fixed by removing one assert check, but I've found several more minors, so may be it worth to revert? I think this should be recommited with negative tests for extractelement as wel as for insertelement (are we able to schedule insertelement with non-constant index now?)

Could you revert the patch and I'll fix it once I'm back from vacation?

anton-afanasyev requested changes to this revision.Aug 31 2021, 5:31 AM

Could you revert the patch and I'll fix it once I'm back from vacation?

Sure!

This revision now requires changes to proceed.Aug 31 2021, 5:31 AM
ABataev added inline comments.Sep 14 2021, 6:53 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
221

No, it is required and checks for a different thing. It checks that the index is constant for such instructions.

234

Because we don't need to check the parents for such instructions. For InsertsElements it was checked already, for Undefs - no need, for Extracts with constant indices - not required.

5959

No, not needed, just isVectorLikeInstWithConstOps(S.OpValue) is enough. Moreover, if we have inserts/extracts with non-const indices, they should be gathered, we can't vectorize them. Just added a special check in BoUpSLP::buildTree_rec function.

6055

Same as above

6095–6097

No, the assert is correct, we should allow extracts with const indices to be scheduled. It is fixed by the mentioned additional check in BoUpSLP::buildTree_rec

ABataev updated this revision to Diff 372478.Sep 14 2021, 7:22 AM

Fixed handling of the inserts/extracts with non-const indices.

Please rebase against llvm/test/Transforms/SLPVectorizer/X86/extract_with_non_const_index.ll commited at aaae726afb0ef.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
221

But you've made check already at line 217, haven't you? Return true for any index if isa<ExtractValueInst>(I).

ABataev added inline comments.Sep 14 2021, 8:25 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
221

Oh, yes, missed that you meant ExtractValueInst. Yes, need to remove the check for ExtractValueInst here, it cannot have non-const indices per LLVM IR specification.

ABataev updated this revision to Diff 372491.Sep 14 2021, 8:35 AM

Removed extra check from the function.

anton-afanasyev accepted this revision.Sep 22 2021, 2:21 PM

Please rebase against llvm/test/Transforms/SLPVectorizer/X86/extract_with_non_const_index.ll commited at aaae726afb0ef.

LGTM with minor concerning rebase.

This revision is now accepted and ready to land.Sep 22 2021, 2:21 PM
This revision was landed with ongoing or failed builds.Sep 28 2021, 6:14 AM
This revision was automatically updated to reflect the committed changes.