This is an archive of the discontinued LLVM Phabricator instance.

[X86] Move promotion of vector and/or/xor from legalization to DAG combine
ClosedPublic

Authored by craig.topper on Oct 10 2018, 1:54 PM.

Details

Summary

I've noticed that the bitcasts we introduce for these make computeKnownBits and computeNumSignBits not work well in LegalizeVectorOps. LegalizeVectorOps legalizes bottom up while LegalizeDAG legalizes top down. The bottom up strategy for LegalizeVectorOps means operands are legalized before their uses. So we promote and/or/xor before we legalize the operands that use them making computeKnownBits/computeNumSignBits in places like LowerTruncate suboptimal. I looked at changing LegalizeVectorOps to be top down as well, but that was more disruptive and caused some regressions. I also looked at just moving promotion of binops to LegalizeDAG, but that had a few issues one around matching AND,ANDN,OR into VSELECT because I had to create ANDN as vXi64, but the other nodes hadn't legalized yet, I didn't look too hard at fixing that.

This patch seems to produce better results overall than my other attempts. We now form broadcasts of constants better in some cases. For at least some of them the AND was being introduced in LegalizeDAG, promoted to vXi64, and the BUILD_VECTOR was also legalized there. I think we got bad ordering of that. Now the promotion is out of the legalizer so we handle this better.

In the longer term I think we really should evaluate whether we should be doing this promotion at all. It's really there to reduce isel pattern count, but I'm wondering if we'd be better served just eating the pattern cost or doing C++ based isel for vector and/or/xor in X86ISelDAGToDAG. The masked and/or/xor will definitely be difficult in patterns if a bitcast gets between the vselect and the and/or/xor node. That becomes a lot of permutations to cover.

Diff Detail

Event Timeline

craig.topper created this revision.Oct 10 2018, 1:54 PM
craig.topper added inline comments.Oct 10 2018, 2:00 PM
test/CodeGen/X86/vector-trunc-usat.ll
734

Looks like we're now using a pcmpeq+xor+shr instead of a andn with a constant pool throughout this test. I wonder if this already happens with vXi64 types and now it happens with other vector types.

Entirely getting rid of the BITCASTs around logic ops (including X86ISD::ANDNP) would be wonderful - we miss a bit of reassociation later on because of this as well.

The only issue I can see is the PANDN -> XOR+PSRL change from allbits/comparison results - is this something we want to fix before/after this patch?

Add a fix to prevent xor+psrl from being selected over andn.

RKSimon accepted this revision.Oct 13 2018, 3:17 PM

LGTM with one minor query - cheers

test/CodeGen/X86/vector-trunc-packus.ll
2095

Any idea what this is?

This revision is now accepted and ready to land.Oct 13 2018, 3:17 PM
This revision was automatically updated to reflect the committed changes.