This is an archive of the discontinued LLVM Phabricator instance.

[CostModel] Treat Identity shuffle masks as zero cost
ClosedPublic

Authored by RKSimon on Jun 9 2018, 1:36 PM.

Details

Summary

As discussed on D47985, identity shuffle masks should probably be free.

I've limited this to the case where the input and output types all match - but we could probably accept all cases.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Jun 9 2018, 1:36 PM

We should fold these in InstSimplify, so assigning a zero cost seems appropriate, but that raises the question: who's creating these? Couldn't they call llvm::SimplifyShuffleVectorInst() and short-circuit the problem?

We should fold these in InstSimplify, so assigning a zero cost seems appropriate, but that raises the question: who's creating these? Couldn't they call llvm::SimplifyShuffleVectorInst() and short-circuit the problem?

I believe this is the default expansion of the reduction intrinsics - I suppose specifying undef in the upper lanes is useful to help propagate demanded elts so InstSimplify might prevent further improvements if it prematurely optimizes?

I know there are a couple of tests in AMDGPU that was specifying an identity shuffle as one of its costs tests (it expects it to be recognised as a SK_PermuteSingleSrc which is free - so this patch isn't affecting it).

We should fold these in InstSimplify, so assigning a zero cost seems appropriate, but that raises the question: who's creating these? Couldn't they call llvm::SimplifyShuffleVectorInst() and short-circuit the problem?

I believe this is the default expansion of the reduction intrinsics - I suppose specifying undef in the upper lanes is useful to help propagate demanded elts so InstSimplify might prevent further improvements if it prematurely optimizes?

I know there are a couple of tests in AMDGPU that was specifying an identity shuffle as one of its costs tests (it expects it to be recognised as a SK_PermuteSingleSrc which is free - so this patch isn't affecting it).

Ah, I forgot about that lack of seeing through undef elts in InstSimplify. So we probably want to handle non-canonical IR in the cost model either way to be safe. Add a test where we're selecting from operand 1 rather than operand 0, so we have some coverage for that case too then?

RKSimon updated this revision to Diff 150906.Jun 12 2018, 2:34 AM

Refreshed with tests for RHS Identity shuffles

spatel accepted this revision.Jun 12 2018, 7:28 AM

LGTM.

This revision is now accepted and ready to land.Jun 12 2018, 7:28 AM
This revision was automatically updated to reflect the committed changes.