Page MenuHomePhabricator

[SelectionDAG] Add initial implementation of TargetLowering::SimplifyDemandedVectorElts

Authored by RKSimon on Feb 4 2018, 1:24 PM.



This is mainly a move of simplifyShuffleOperands from DAGCombiner::visitVECTOR_SHUFFLE to create a more general purpose TargetLowering::SimplifyDemandedVectorElts implementation.

There are several features that I've begun to implement but haven't made use of yet - the KnownUndef/KnownZero element masks and support for SimplifyDemandedVectorEltsForTargetNode, if people want I can remove these for now.

Diff Detail


Event Timeline

RKSimon created this revision.Feb 4 2018, 1:24 PM
sdardis added inline comments.Feb 6 2018, 5:40 AM
1303 ↗(On Diff #132780)

Nit: getBitWidth() returns an unsigned int, leading to a sign compare warning in the assert below.

1331 ↗(On Diff #132780)

Nit: getScalarSizeInBits returns an unsigned int causing a -Wsign-compare warning at line 1365.

53 ↗(On Diff #132780)

These MIPS changes come about about the MIPS backend doesn't perform custom lowering of BUILD_VECTOR nodes which have undef elements. (See lib/Target/Mips/MipsSEISelLowering.cpp:2377)

As a result that node undergoes the normal expansion which places the elements on the stack aligned to the size of the vector register. However, MIPS's O32 ABI only provides a stack alignment of 8 rather than 16. This causes dynamic stack realignment.

A quick modification in the MIPS backend to isConstantOfUndef and lowerBUILD_VECTOR to lift that restriction shows a more reasonable code sequence with this patch. I have yet to perform further testing of this.

RKSimon updated this revision to Diff 133769.Feb 10 2018, 1:53 PM

Fixed signed/unsigned mismatch

@sdardis Are you wanting to fix the mips buildvector undef handling before this patch can land?

I'll address the size regression after this lands. It's an odd enough case that I don't feel its worth holding this patch up.

zvi added inline comments.Feb 14 2018, 6:45 AM
1345 ↗(On Diff #133769)

Another signed/unsigned compare mismatch

1403 ↗(On Diff #133769)

signed/unsigned compare mismatch

1425 ↗(On Diff #133769)

here too

zvi added inline comments.Feb 14 2018, 7:21 AM
1317 ↗(On Diff #133769)

Is this case is covered by tests?

1393 ↗(On Diff #133769)

Just wondering how an existing VECTOR_SHUFFLE DAGCombiner simplification, such as shuffle(splat) -> splat, relates to this analysis.
Would it be more powerful to move it here? Should the existing DAGCombiner simplification use this analysis or neither because this analysis+simplification will indirectly make the DAGCombiner simplification more powerful?

28192 ↗(On Diff #133769)

Sorry if i'm missing here something obvious, but is this overload needed?

RKSimon marked 2 inline comments as done.Feb 14 2018, 12:40 PM

Finally gotten back to a non-MSVC build machine, I'll get those signed/unsigned warning fixed up properly in the next diff.

1317 ↗(On Diff #133769)

IIRC yes - I think it gets created in the sse3.ll test that changes.

1393 ↗(On Diff #133769)

Yes, a number of those combine could possibly be moved. I've avoided doing too much that extends beyond existing features as they can have a large effect on x86 shuffle tests, for instance simplifying shuffle masks in here causes a number of regressions that I'm still triaging.

28192 ↗(On Diff #133769)

It was mainly added as part of tests while developing this patch - as I mentioned in the summary I'm happy to remove it from this patch, but I intend to add it in a future patch quite quickly.

zvi accepted this revision.Feb 14 2018, 1:10 PM

LGTM after fixing the signed/unsigned mismatches.

This revision is now accepted and ready to land.Feb 14 2018, 1:10 PM
This revision was automatically updated to reflect the committed changes.