Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[IR]Add NumSrcElts param to is..Mask static function in ShuffleVectorInst.
AcceptedPublic

Authored by ABataev on Aug 21 2023, 1:05 PM.

Details

Reviewers
RKSimon
Summary

Need to add NumSrcElts param to is..Mask functions in
ShuffleVectorInstruction class for better mask analysis. Mask.size() not
always matches the sizes of the permuted vector(s). Allows to better
estimate the cost in SLP and fix uses of the functions in other cases.

Diff Detail

Event Timeline

ABataev created this revision.Aug 21 2023, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 1:05 PM
ABataev requested review of this revision.Aug 21 2023, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 1:05 PM
craig.topper added inline comments.
llvm/lib/IR/Instructions.cpp
2178–2179

If Mask.size() == 4 and NumSrcElts == 6. Then this considers <5, 4, 3, 2> as a reverse?

ABataev added inline comments.Aug 21 2023, 1:52 PM
llvm/lib/IR/Instructions.cpp
2178–2179

Yes. Need to add a check that the operation does not change the size, just like non-static isReverse() does.

ABataev updated this revision to Diff 552154.Aug 21 2023, 3:19 PM

Address comments

RKSimon added inline comments.Aug 23 2023, 1:46 AM
llvm/include/llvm/IR/Instructions.h
2070–2071

Update this comment

2165–2166

Update comment

2186–2187

Update comment

2259

Update comment

llvm/unittests/IR/ShuffleVectorInstTest.cpp
14

All of these tests need support for length changing shuffles

ABataev updated this revision to Diff 552703.Aug 23 2023, 7:25 AM

Address comments

craig.topper added inline comments.Wed, Sep 6, 8:58 AM
llvm/lib/IR/Instructions.cpp
2158

Why is the argument an int if we're going to cast it to unsigned?

ABataev added inline comments.Wed, Sep 6, 9:48 AM
llvm/lib/IR/Instructions.cpp
2158

For conformance. All other functions have corresponding int parameter, though treat it as unsigned.

ABataev updated this revision to Diff 556478.Mon, Sep 11, 1:23 PM

Rebase, ping!

ABataev updated this revision to Diff 557058.Tue, Sep 19, 10:35 AM

Rebase, ping!

No objections to this, but I do think it'd make sense to split into 2 patches - (1) adding the NumSrcElts argument to the shuffle matching helpers + additional unit test coverage and (2) using it in SLP

ABataev updated this revision to Diff 557193.Thu, Sep 21, 12:08 PM

Rebase, address comments

RKSimon added inline comments.Tue, Sep 26, 12:15 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6658

Maybe split this into a series of separate if () { return true/false; } to make it easier to grok?

ABataev updated this revision to Diff 557371.Tue, Sep 26, 12:53 PM

Address comment

ABataev marked an inline comment as done.Tue, Sep 26, 12:58 PM
ABataev added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6658

Done

ABataev marked an inline comment as done.Tue, Sep 26, 1:00 PM
RKSimon accepted this revision.Wed, Sep 27, 3:01 AM

LGTM - cheers

This revision is now accepted and ready to land.Wed, Sep 27, 3:01 AM