This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add a DAG combine to turn vector (and (srl X, ((1 << C1) - 1)), C2) into (srl (shl (X, C3), C4)) to save a constant pool for the AND mask
Needs ReviewPublic

Authored by craig.topper on Aug 21 2019, 5:34 PM.

Details

Summary

If we're already shifting right we can trade the AND for a SHL to clear out the upper bits before the shift right. This can avoid a constant pool on the AND.

This patch doesn't currently work for AND's with power 2 since those get turned into shuffles and only become ANDs again during the last lowering step. At which point the mask is in a constant pool. We can probably look through the constant pool to fix this, but I haven't looked at that yet.

Diff Detail

Event Timeline

craig.topper created this revision.Aug 21 2019, 5:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 5:34 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

We already have these in TLI:

TLI::shouldFoldConstantShiftPairToMask
TLI::shouldFoldMaskToVariableShiftPair

ideally we'd keep everything in DAGCombine and have a suitable set of TLI hooks to indicate what codegen we prefer

We already have these in TLI:

TLI::shouldFoldConstantShiftPairToMask
TLI::shouldFoldMaskToVariableShiftPair

ideally we'd keep everything in DAGCombine and have a suitable set of TLI hooks to indicate what codegen we prefer

+1 to that, this was the patch i was going to look into after D62100 but that one got stuck.
Which means, this doesn't really have to be X86-specific.

lebedev.ri requested changes to this revision.Aug 31 2019, 4:25 AM

(Just marking as reviewed) Based on previous disscussion let me know if i should try to take this over.

This revision now requires changes to proceed.Aug 31 2019, 4:25 AM

@craig.topper Whatever happened to this patch? It still looks useful.

I don't think I ever did anything more with this. @lebedev.ri did you want to pick this up?

I don't think I ever did anything more with this. @lebedev.ri did you want to pick this up?

I wanted to, and to put this into DAGCombiner under target hooks,
but i thought about doing that after D62100, which is stuck due to AArch64 failures to form ubfx.
But now that D62100 is happening in instcombine, i'm not so sure we need it?

So i guess i can try picking this up.

lkail added a subscriber: lkail.Jan 12 2021, 3:40 AM
lebedev.ri resigned from this revision.Jan 12 2023, 4:51 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

This revision now requires review to proceed.Jan 12 2023, 4:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 4:51 PM
Herald added a subscriber: StephenFan. · View Herald Transcript