Page MenuHomePhabricator

[X86] Convert X86ISD::SELECT to ISD::VSELECT just before instruction selection to avoid duplicate patterns

Authored by craig.topper on Sep 18 2017, 10:29 PM.



Similar to what we do for X86ISD::SHRUNKBLEND just turn X86ISD::SELECT into ISD::VSELECT. This allows us to remove the duplicated TRUNC patterns.

Diff Detail


Event Timeline

craig.topper created this revision.Sep 18 2017, 10:29 PM
delena edited edge metadata.Sep 18 2017, 11:26 PM

Please add a test.

What sort of test are you looking for? The truncate intrinsic tests should already cover this.

Ok. This is NFC. Why we can't generate (or stay with) ISD::SELECT from the beginning?

Here's the comment from getVectorMaskingNode

case X86ISD::VTRUNC:
case X86ISD::CVTPS2PH:
  // We can't use ISD::VSELECT here because it is not always "Legal"
  // for the destination type. For example vpmovqb require only AVX512
  // and vselect that can operate on byte element type require BWI
  OpcodeSelect = X86ISD::SELECT;

Yes, I saw this comment. We create a custom node in order to avoid an additional "entrance" to LowerSELECT(), right?
In general, this approach is generic - create a custom node to stop the lowering process.
If we'll keep a table of custom-to-common last-minute replacement, it'd be, probably, more robust.
It may be implemented later.

X86select node is not used any more, it can be removed from td patterns.

I did remove the usages from the td patterns. It wasn't used in very many places.

The true side of the ternary in this code also looks dead. We never get this far for vector types. So we never select X86ISD::SELECT. But its not strictly related to the goal of this patch.

if (Subtarget.hasAVX512()) {
  SDValue Cmp = DAG.getNode(X86ISD::FSETCCM, DL, MVT::v1i1, CondOp0,
                            CondOp1, DAG.getConstant(SSECC, DL, MVT::i8));
  return DAG.getNode(VT.isVector() ? X86ISD::SELECT : X86ISD::SELECTS,
                     DL, VT, Cmp, Op1, Op2);

I did remove the usages from the td patterns. It wasn't used in very many places.

I meant

Remove from

delena accepted this revision.Sep 19 2017, 12:54 AM
This revision is now accepted and ready to land.Sep 19 2017, 12:54 AM
This revision was automatically updated to reflect the committed changes.