[PATCH] D66687: [x86] try to form more bt/test + set out of shift+mask patterns

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added a comment.

In D66687#1644003 <https://reviews.llvm.org/D66687#1644003>, @lebedev.ri wrote:

> I would think this should go into DAGCombiner under `hasBitTest()` hook.


Yes, I forgot we have that hook. Hexagon has enabled it as well as x86, so I added some more tests.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:38981
+  // Look through an optional extension and find a 'not'.
+  // TODO: Should we favor test+set even without the 'not' op?
+  SDValue Not = And->getOperand(0), And1 = And->getOperand(1);
----------------
craig.topper wrote:
> lebedev.ri wrote:
> > I'd think we should, it should just change the predicate i think?
> > 
> Without the not we don’t save any instructions do we? Setcc also has lower throughput on recent Intel CPUs and has a partial register update..
In all the tests I looked at, we avoid the partial reg update problem by xor'ing the reg. But yes, without the 'not' instruction, this is trading 2 instructions for 2 instructions that might have less throughput.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:39006
+  APInt OneBit =
+      APInt::getOneBitSet(VT.getSizeInBits(), Srl.getConstantOperandVal(1));
+  SDValue BitMask = DAG.getConstant(OneBit, DL, VT);
----------------
craig.topper wrote:
> How do we know the shift amount isn't bigger than the bit width?
Yes, good catch. I think we're safe in the basic case without trunc/ext (all oversized shifts get folded to undef), but since we're potentially truncating the input value, we either need to guard against that or add some logic to decide what width we should do the and/setcc. I'll add a bailout for now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66687/new/

https://reviews.llvm.org/D66687