Page MenuHomePhabricator

X86: Utilize ZeroableElements for canWidenShuffleElements
ClosedPublic

Authored by RKSimon on Jan 14 2018, 3:47 PM.

Details

Summary

canWidenShuffleElements can do a better job if given a mask with ZeroableElements info. Apparently, ZeroableElements was being only used to identify AllZero candidates, but possibly we could plug it into more shuffle matchers.

Diff Detail

Repository
rL LLVM

Event Timeline

zvi created this revision.Jan 14 2018, 3:47 PM
RKSimon added inline comments.Jan 19 2018, 7:00 AM
lib/Target/X86/X86ISelLowering.cpp
14401 ↗(On Diff #129794)

ISD::isBuildVectorAllZeros allows undef elements (as long as it isn't all undefs) - should we test for a splat build vector of zero and check that we have no undefs at all?

zvi added inline comments.Jan 19 2018, 9:08 AM
lib/Target/X86/X86ISelLowering.cpp
14401 ↗(On Diff #129794)

What would be the benefit of that? I mean, undefs can be zeroed if profitable.

zvi updated this revision to Diff 132777.Feb 4 2018, 11:42 AM

Rebase + ping

RKSimon added inline comments.Feb 7 2018, 5:53 AM
lib/Target/X86/X86ISelLowering.cpp
14571 ↗(On Diff #132777)

Are there any cases where the zero entry came from V1 that could cause a problem?

14595 ↗(On Diff #132777)

APInt Undefs = APInt::getAllOnesValue(NewNumElts);

14597 ↗(On Diff #132777)

APInt::getNullValue(NewEltVT.getSizeInBits())

14600 ↗(On Diff #132777)

i > NewNumElts can't ever be true? And anyway, we expect the zero to have come from V2.

zvi updated this revision to Diff 141575.Apr 8 2018, 4:37 PM
  • Rebase
  • Simplify the creation of the shuffle by only modifying the widened mask to take all zeros from the all-zero operand.
zvi marked 3 inline comments as done.Apr 8 2018, 4:46 PM
zvi added inline comments.
lib/Target/X86/X86ISelLowering.cpp
14571 ↗(On Diff #132777)

Sorry i didn't understand the question.

14595 ↗(On Diff #132777)

Thanks, this code was removed from latest revision

14597 ↗(On Diff #132777)

Thanks, this code was removed from latest revision

14600 ↗(On Diff #132777)

whoops. I fixed this in the latest revision

@zvi Do you mind if I commandeer this and get it completed?

RKSimon commandeered this revision.Jul 8 2018, 12:05 PM
RKSimon edited reviewers, added: zvi; removed: RKSimon.

Commandeering as @zvi is busy with other things

RKSimon updated this revision to Diff 155143.Jul 12 2018, 3:26 AM
RKSimon marked 3 inline comments as done.
RKSimon edited the summary of this revision. (Show Details)
RKSimon added reviewers: spatel, andreadb.

Rebased and updated to fix potential issue if whole of V2 isn't zeros

Rebased and updated to fix potential issue if whole of V2 isn't zeros

Looks good to me. Thanks Simon!

andreadb accepted this revision.Jul 12 2018, 6:08 AM
This revision is now accepted and ready to land.Jul 12 2018, 6:08 AM
This revision was automatically updated to reflect the committed changes.