[X86][BMI][TBM] Only demand bottom 16-bits of the BEXTR control op (PR34042)

[RKSimon]

Summary

Only the bottom 16-bits of BEXTR's control op are required (0:8 INDEX, 15:8 LENGTH).

[craig.topper]

lib/Target/X86/X86InstrCompiler.td

2047

Why aren't these load patterns redundant now?

[lebedev.ri]

lib/Target/X86/X86InstrCompiler.td

2051

Why SExt; ahouldn't ZExt be fine, especially since only low 16 bits will be used?

[craig.topper]

lib/Target/X86/X86InstrCompiler.td

2051

Its consistent with the behavior of the gnu assembler and basically any other 64 bit x86 instruction that only encodes a 32 bit immediate. Doing something else would require a new assembly parser method specifically for this instruction.

[craig.topper]

Hilariously, the tbm-builtins.c test in clang fails -O0 isel on one of the TBM bextr tests because the immediate is too large.

[RKSimon]

Remove remaining (unnecessary) TBM X86bextr patterns

[RKSimon]

In D47690#1120298, @craig.topper wrote:

Hilariously, the tbm-builtins.c test in clang fails -O0 isel on one of the TBM bextr tests because the immediate is too large.

I'm not seeing this with the latest version of my patch - do you still have a repro?

[craig.topper]

It fails on trunk. Your patch probably fixes it by shrinking the immediate. Is there anything that can prevent the DAG combine from running that we should be concerned about? If there is we might not need to clamp the immediate in lowering or isel.

[RKSimon]

In D47690#1120998, @craig.topper wrote:

It fails on trunk. Your patch probably fixes it by shrinking the immediate. Is there anything that can prevent the DAG combine from running that we should be concerned about? If there is we might not need to clamp the immediate in lowering or isel.

I don't think TargetLowering::SimplifyDemandedBits alters constants so I might not be repro'ing it properly - my X86ISelDAGToDAG.cpp change might be causing it to fallback to the reg-reg version?

What was the full repro command line please?

[craig.topper]

Ok its not the exact tbm-builtins.c test after all I think I had modified it when I was testing. This should fail

#include <x86intrin.h>

unsigned long long test__bextri_u64_bigint(unsigned long long a) {
  return __bextri_u64(a, 0x7ffffff00LL);
}

[craig.topper]

Well 0x7ffffffffLL fails with -O0. https://godbolt.org/g/AC16ui I had to pick 6.0.0 because trunk segfaulted on anything, but I think that must be a problem with godbolt itself.

[RKSimon]

Perform a demanded bits style mask on BEXTR constant masks to fix the existing i32 sign-extend isel issue

[craig.topper]

LGTM. I'll let you decide what to do with the APInts.

lib/Target/X86/X86ISelLowering.cpp

36835

nit. this can be const APInt &Val.

36836

You can use Val1 & 0xffff if you want. There's an operator& that takes an APInt and uint64_t.

Or you could do something like.

if (Val1.ugt(0xFFFF)
  return DAG.getNode(X86ISD::BEXTR, SDLoc(N), VT, Op0,
                         DAG.getConstant(Val1 & 0xffff, SDLoc(N), VT));

lib/Target/X86/X86ISelDAGToDAG.cpp

if (const ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N)) {
     if (!isUInt<32>(ImmVal))
       return false;
 
     Imm = CurDAG->getTargetConstant(ImmVal, SDLoc(N), MVT::i64);
     return true;
   }
 
   // In static codegen with small code model, we can get the address of a label
-  // into a register with 'movl'. TableGen has already made sure we're looking
-  // at a label of some kind.
-  assert(N->getOpcode() == X86ISD::Wrapper &&
-         "Unexpected node type for MOV32ri64");
+  // into a register with 'movl'
+  if (N->getOpcode() != X86ISD::Wrapper)
+    return false;
+
   N = N.getOperand(0);
 
   // At least GNU as does not accept 'movl' for TPOFF relocations.
   // FIXME: We could use 'movl' when we know we are targeting MC.
   if (N->getOpcode() == ISD::TargetGlobalTLSAddress)
     return false;
 
   Imm = N;

lib/Target/X86/X86ISelLowering.cpp

static SDValue combineXor(SDNode *N, SelectionDAG &DAG,
   if (SDValue FPLogic = convertIntLogicToFPLogic(N, DAG, Subtarget))
     return FPLogic;
 
   if (isFNEG(N))
     return combineFneg(N, DAG, Subtarget);
   return SDValue();
 }
 
+static SDValue combineBEXTR(SDNode *N, SelectionDAG &DAG,
+                            TargetLowering::DAGCombinerInfo &DCI,
+                            const X86Subtarget &Subtarget) {
+  SDValue Op0 = N->getOperand(0);
+  SDValue Op1 = N->getOperand(1);
+  EVT VT = N->getValueType(0);
+  unsigned NumBits = VT.getSizeInBits();
+
+  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+  TargetLowering::TargetLoweringOpt TLO(DAG, !DCI.isBeforeLegalize(),
+                                        !DCI.isBeforeLegalizeOps());
+
+  // TODO - Constant Folding.
+  if (auto *Cst1 = dyn_cast<ConstantSDNode>(Op1)) {
+    // Reduce Cst1 to the bottom 16-bits.
+    // NOTE: SimplifyDemandedBits won't do this for constants.
+    APInt Val1 = Cst1->getAPIntValue();
+    APInt MaskedVal1 = Val1 & APInt(NumBits, 0xFFFF);
+    if (MaskedVal1 != Val1)
+      return DAG.getNode(X86ISD::BEXTR, SDLoc(N), VT, Op0,
+                         DAG.getConstant(MaskedVal1, SDLoc(N), VT));
+  }
+
+  // Only bottom 16-bits of the control bits are required.
+  KnownBits Known;
+  APInt DemandedMask(APInt::getLowBitsSet(NumBits, 16));
+  if (TLI.SimplifyDemandedBits(Op1, DemandedMask, Known, TLO)) {
+    DCI.CommitTargetLoweringOpt(TLO);
+    return SDValue(N, 0);
+  }
+
+  return SDValue();
+}
 
 static bool isNullFPScalarOrVectorConst(SDValue V) {
   return isNullFPConstant(V) || ISD::isBuildVectorAllZeros(V.getNode());
 }
 
 /// If a value is a scalar FP zero or a vector FP zero (potentially including
 /// undefined elements), return a zero constant that may be used to fold away
 /// that value. In the case of a vector, the returned constant will not contain
SDValue X86TargetLowering::PerformDAGCombine(SDNode *N,
   case X86ISD::ADC:         return combineADC(N, DAG, DCI);
   case ISD::MUL:            return combineMul(N, DAG, DCI, Subtarget);
   case ISD::SHL:
   case ISD::SRA:
   case ISD::SRL:            return combineShift(N, DAG, DCI, Subtarget);
   case ISD::AND:            return combineAnd(N, DAG, DCI, Subtarget);
   case ISD::OR:             return combineOr(N, DAG, DCI, Subtarget);
   case ISD::XOR:            return combineXor(N, DAG, DCI, Subtarget);
+  case X86ISD::BEXTR:       return combineBEXTR(N, DAG, DCI, Subtarget);
   case ISD::LOAD:           return combineLoad(N, DAG, DCI, Subtarget);
   case ISD::MLOAD:          return combineMaskedLoad(N, DAG, DCI, Subtarget);
   case ISD::STORE:          return combineStore(N, DAG, Subtarget);
   case ISD::MSTORE:         return combineMaskedStore(N, DAG, Subtarget);
   case ISD::SINT_TO_FP:     return combineSIntToFP(N, DAG, Subtarget);
   case ISD::UINT_TO_FP:     return combineUIntToFP(N, DAG, Subtarget);
   case ISD::FADD:
   case ISD::FSUB:           return combineFaddFsub(N, DAG, Subtarget);

lib/Target/X86/X86InstrCompiler.td

def : Pat<(X86bextr GR64:$src1, mov64imm32:$src2),
                                       (MOV32ri64 mov64imm32:$src2),
                                       sub_32bit))>;
   def : Pat<(X86bextr (loadi64 addr:$src1), mov64imm32:$src2),
             (BEXTR64rm addr:$src1,
                        (SUBREG_TO_REG (i64 0),
                                       (MOV32ri64 mov64imm32:$src2),
                                       sub_32bit))>;
 } // HasBMI, NoTBM
-
-let Predicates = [HasTBM] in {
-  def : Pat<(X86bextr GR32:$src1, (i32 imm:$src2)),
-            (BEXTRI32ri GR32:$src1, imm:$src2)>;
-  def : Pat<(X86bextr (loadi32 addr:$src1), (i32 imm:$src2)),
-            (BEXTRI32mi addr:$src1, imm:$src2)>;
-  def : Pat<(X86bextr GR64:$src1, i64immSExt32:$src2),
-            (BEXTRI64ri GR64:$src1, i64immSExt32:$src2)>;
-  def : Pat<(X86bextr (loadi64 addr:$src1), i64immSExt32:$src2),
-            (BEXTRI64mi addr:$src1, i64immSExt32:$src2)>;
-}

lib/Target/X86/X86InstrInfo.td

def : Pat<(xor GR64:$src, (add GR64:$src, -1)),
             (BLSMSK64rr GR64:$src)>;
 
   def : Pat<(and GR32:$src, (ineg GR32:$src)),
             (BLSI32rr GR32:$src)>;
   def : Pat<(and GR64:$src, (ineg GR64:$src)),
             (BLSI64rr GR64:$src)>;
 }
 
-multiclass bmi_bextr_bzhi<bits<8> opc, string mnemonic, RegisterClass RC,
-                          X86MemOperand x86memop, Intrinsic Int,
-                          PatFrag ld_frag, X86FoldableSchedWrite Sched> {
+multiclass bmi_bextr<bits<8> opc, string mnemonic, RegisterClass RC,
+                     X86MemOperand x86memop, SDNode OpNode,
   def rr : I<opc, MRMSrcReg4VOp3, (outs RC:$dst), (ins RC:$src1, RC:$src2),
              !strconcat(mnemonic, "\t{$src2, $src1, $dst|$dst, $src1, $src2}"),
-             [(set RC:$dst, (Int RC:$src1, RC:$src2)), (implicit EFLAGS)]>,
+             [(set RC:$dst, (OpNode RC:$src1, RC:$src2)), (implicit EFLAGS)]>,
              T8PS, VEX, Sched<[Sched]>;
   def rm : I<opc, MRMSrcMem4VOp3, (outs RC:$dst), (ins x86memop:$src1, RC:$src2),
              !strconcat(mnemonic, "\t{$src2, $src1, $dst|$dst, $src1, $src2}"),
-             [(set RC:$dst, (Int (ld_frag addr:$src1), RC:$src2)),
+             [(set RC:$dst, (OpNode (ld_frag addr:$src1), RC:$src2)),
               (implicit EFLAGS)]>, T8PS, VEX,
              Sched<[Sched.Folded,
                     // x86memop:$src1
                     ReadDefault, ReadDefault, ReadDefault, ReadDefault,
                     ReadDefault,
                     // RC:$src2
                     ReadAfterLd]>;
 }
 
 let Predicates = [HasBMI], Defs = [EFLAGS] in {
-  defm BEXTR32 : bmi_bextr_bzhi<0xF7, "bextr{l}", GR32, i32mem,
-                                int_x86_bmi_bextr_32, loadi32, WriteBEXTR>;
-  defm BEXTR64 : bmi_bextr_bzhi<0xF7, "bextr{q}", GR64, i64mem,
-                                int_x86_bmi_bextr_64, loadi64, WriteBEXTR>, VEX_W;
+  defm BEXTR32 : bmi_bextr<0xF7, "bextr{l}", GR32, i32mem,
+                           X86bextr, loadi32, WriteBEXTR>;
+  defm BEXTR64 : bmi_bextr<0xF7, "bextr{q}", GR64, i64mem,
+                           X86bextr, loadi64, WriteBEXTR>, VEX_W;
+}
+
+multiclass bmi_bzhi<bits<8> opc, string mnemonic, RegisterClass RC,
+                    X86MemOperand x86memop, Intrinsic Int,
+                    PatFrag ld_frag, X86FoldableSchedWrite Sched> {
+  def rr : I<opc, MRMSrcReg4VOp3, (outs RC:$dst), (ins RC:$src1, RC:$src2),
+             !strconcat(mnemonic, "\t{$src2, $src1, $dst|$dst, $src1, $src2}"),
+             [(set RC:$dst, (Int RC:$src1, RC:$src2)), (implicit EFLAGS)]>,
+             T8PS, VEX, Sched<[Sched]>;
+  def rm : I<opc, MRMSrcMem4VOp3, (outs RC:$dst), (ins x86memop:$src1, RC:$src2),
+             !strconcat(mnemonic, "\t{$src2, $src1, $dst|$dst, $src1, $src2}"),
+             [(set RC:$dst, (Int (ld_frag addr:$src1), RC:$src2)),
+              (implicit EFLAGS)]>, T8PS, VEX,
+             Sched<[Sched.Folded,
+                    // x86memop:$src1
+                    ReadDefault, ReadDefault, ReadDefault, ReadDefault,
+                    ReadDefault,
+                    // RC:$src2
+                    ReadAfterLd]>;
 }
 
 let Predicates = [HasBMI2], Defs = [EFLAGS] in {
-  defm BZHI32 : bmi_bextr_bzhi<0xF5, "bzhi{l}", GR32, i32mem,
-                               int_x86_bmi_bzhi_32, loadi32, WriteBZHI>;
-  defm BZHI64 : bmi_bextr_bzhi<0xF5, "bzhi{q}", GR64, i64mem,
-                               int_x86_bmi_bzhi_64, loadi64, WriteBZHI>, VEX_W;
+  defm BZHI32 : bmi_bzhi<0xF5, "bzhi{l}", GR32, i32mem,
+  defm BZHI64 : bmi_bzhi<0xF5, "bzhi{q}", GR64, i64mem,
 }
 
 def CountTrailingOnes : SDNodeXForm<imm, [{
   // Count the trailing ones in the immediate.
   return getI8Imm(countTrailingOnes(N->getZExtValue()), SDLoc(N));
 }]>;
 
 def BEXTRMaskXForm : SDNodeXForm<imm, [{
defm PEXT64 : bmi_pdep_pext<"pext{q}", GR64, i64mem,
                                int_x86_bmi_pext_64, loadi64>, T8XS, VEX_W;
 }
 
 //===----------------------------------------------------------------------===//
 // TBM Instructions
 //
 let Predicates = [HasTBM], Defs = [EFLAGS] in {
 
-multiclass tbm_ternary_imm_intr<bits<8> opc, RegisterClass RC, string OpcodeStr,
-                                X86MemOperand x86memop, PatFrag ld_frag,
-                                Intrinsic Int, Operand immtype,
-                                SDPatternOperator immoperator,
-                                X86FoldableSchedWrite Sched> {
+multiclass tbm_ternary_imm<bits<8> opc, RegisterClass RC, string OpcodeStr,
+                           SDNode OpNode, Operand immtype,
   def ri : Ii32<opc,  MRMSrcReg, (outs RC:$dst), (ins RC:$src1, immtype:$cntl),
                 !strconcat(OpcodeStr,
                            "\t{$cntl, $src1, $dst|$dst, $src1, $cntl}"),
-                [(set RC:$dst, (Int RC:$src1, immoperator:$cntl))]>,
+                [(set RC:$dst, (OpNode RC:$src1, immoperator:$cntl))]>,
                 XOP, XOPA, Sched<[Sched]>;
   def mi : Ii32<opc,  MRMSrcMem, (outs RC:$dst),
                 (ins x86memop:$src1, immtype:$cntl),
                 !strconcat(OpcodeStr,
                            "\t{$cntl, $src1, $dst|$dst, $src1, $cntl}"),
-                [(set RC:$dst, (Int (ld_frag addr:$src1), immoperator:$cntl))]>,
+                [(set RC:$dst, (OpNode (ld_frag addr:$src1), immoperator:$cntl))]>,
                 XOP, XOPA, Sched<[Sched.Folded]>;
 }
 
-defm BEXTRI32 : tbm_ternary_imm_intr<0x10, GR32, "bextr{l}", i32mem, loadi32,
-                                     int_x86_tbm_bextri_u32, i32imm, imm,
-                                     WriteBEXTR>;
+defm BEXTRI32 : tbm_ternary_imm<0x10, GR32, "bextr{l}", i32mem, loadi32,
+                                X86bextr, i32imm, imm, WriteBEXTR>;
 let ImmT = Imm32S in
-defm BEXTRI64 : tbm_ternary_imm_intr<0x10, GR64, "bextr{q}", i64mem, loadi64,
-                                     int_x86_tbm_bextri_u64, i64i32imm,
-                                     i64immSExt32, WriteBEXTR>, VEX_W;
+defm BEXTRI64 : tbm_ternary_imm<0x10, GR64, "bextr{q}", i64mem, loadi64,
+                                X86bextr, i64i32imm,
 
 multiclass tbm_binary_rm<bits<8> opc, Format FormReg, Format FormMem,
                          RegisterClass RC, string OpcodeStr,
                          X86MemOperand x86memop, X86FoldableSchedWrite Sched> {
 let hasSideEffects = 0 in {
   def rr : I<opc,  FormReg, (outs RC:$dst), (ins RC:$src),
              !strconcat(OpcodeStr,"\t{$src, $dst|$dst, $src}"), []>,
              XOP_4V, XOP9, Sched<[Sched]>;

lib/Target/X86/X86IntrinsicsInfo.h

static const IntrinsicData  IntrinsicsWithoutChain[] = {
   X86_INTRINSIC_DATA(avx512_vpermilvar_pd_512, INTR_TYPE_2OP, X86ISD::VPERMILPV, 0),
   X86_INTRINSIC_DATA(avx512_vpermilvar_ps_512, INTR_TYPE_2OP, X86ISD::VPERMILPV, 0),
   X86_INTRINSIC_DATA(avx512_vpmadd52h_uq_128 , IFMA_OP, X86ISD::VPMADD52H, 0),
   X86_INTRINSIC_DATA(avx512_vpmadd52h_uq_256 , IFMA_OP, X86ISD::VPMADD52H, 0),
   X86_INTRINSIC_DATA(avx512_vpmadd52h_uq_512 , IFMA_OP, X86ISD::VPMADD52H, 0),
   X86_INTRINSIC_DATA(avx512_vpmadd52l_uq_128 , IFMA_OP, X86ISD::VPMADD52L, 0),
   X86_INTRINSIC_DATA(avx512_vpmadd52l_uq_256 , IFMA_OP, X86ISD::VPMADD52L, 0),
   X86_INTRINSIC_DATA(avx512_vpmadd52l_uq_512 , IFMA_OP, X86ISD::VPMADD52L, 0),
+  X86_INTRINSIC_DATA(bmi_bextr_32,         INTR_TYPE_2OP, X86ISD::BEXTR, 0),
+  X86_INTRINSIC_DATA(bmi_bextr_64,         INTR_TYPE_2OP, X86ISD::BEXTR, 0),
   X86_INTRINSIC_DATA(fma_vfmadd_pd,        INTR_TYPE_3OP, ISD::FMA, 0),
   X86_INTRINSIC_DATA(fma_vfmadd_pd_256,    INTR_TYPE_3OP, ISD::FMA, 0),
   X86_INTRINSIC_DATA(fma_vfmadd_ps,        INTR_TYPE_3OP, ISD::FMA, 0),
   X86_INTRINSIC_DATA(fma_vfmadd_ps_256,    INTR_TYPE_3OP, ISD::FMA, 0),
   X86_INTRINSIC_DATA(fma_vfmadd_sd,        INTR_TYPE_3OP, X86ISD::FMADDS1, 0),
   X86_INTRINSIC_DATA(fma_vfmadd_ss,        INTR_TYPE_3OP, X86ISD::FMADDS1, 0),
   X86_INTRINSIC_DATA(fma_vfmaddsub_pd,     INTR_TYPE_3OP, X86ISD::FMADDSUB, 0),
   X86_INTRINSIC_DATA(fma_vfmaddsub_pd_256, INTR_TYPE_3OP, X86ISD::FMADDSUB, 0),

   X86_INTRINSIC_DATA(sse4a_insertqi,    INTR_TYPE_4OP, X86ISD::INSERTQI, 0),
   X86_INTRINSIC_DATA(ssse3_phadd_d_128, INTR_TYPE_2OP, X86ISD::HADD, 0),
   X86_INTRINSIC_DATA(ssse3_phadd_w_128, INTR_TYPE_2OP, X86ISD::HADD, 0),
   X86_INTRINSIC_DATA(ssse3_phsub_d_128, INTR_TYPE_2OP, X86ISD::HSUB, 0),
   X86_INTRINSIC_DATA(ssse3_phsub_w_128, INTR_TYPE_2OP, X86ISD::HSUB, 0),
   X86_INTRINSIC_DATA(ssse3_pmadd_ub_sw_128, INTR_TYPE_2OP, X86ISD::VPMADDUBSW, 0),
   X86_INTRINSIC_DATA(ssse3_pmul_hr_sw_128, INTR_TYPE_2OP, X86ISD::MULHRS, 0),
   X86_INTRINSIC_DATA(ssse3_pshuf_b_128, INTR_TYPE_2OP, X86ISD::PSHUFB, 0),
+  X86_INTRINSIC_DATA(tbm_bextri_u32,    INTR_TYPE_2OP, X86ISD::BEXTR, 0),
+  X86_INTRINSIC_DATA(tbm_bextri_u64,    INTR_TYPE_2OP, X86ISD::BEXTR, 0),
   X86_INTRINSIC_DATA(vcvtph2ps_128,     INTR_TYPE_1OP, X86ISD::CVTPH2PS, 0),
   X86_INTRINSIC_DATA(vcvtph2ps_256,     INTR_TYPE_1OP, X86ISD::CVTPH2PS, 0),
   X86_INTRINSIC_DATA(vcvtps2ph_128,     INTR_TYPE_2OP, X86ISD::CVTPS2PH, 0),
   X86_INTRINSIC_DATA(vcvtps2ph_256,     INTR_TYPE_2OP, X86ISD::CVTPS2PH, 0),
 
   X86_INTRINSIC_DATA(vgf2p8affineinvqb_128, INTR_TYPE_3OP,
                      X86ISD::GF2P8AFFINEINVQB, 0),
   X86_INTRINSIC_DATA(vgf2p8affineinvqb_256, INTR_TYPE_3OP,

test/CodeGen/X86/bmi-x86_64.ll

; CHECK-NEXT:    retq
   %3 = and i64 %2, 4095
   ret i64 %3
 }
 
 ; PR34042
 define i64 @bextr64c(i64 %x, i32 %y) {
 ; CHECK-LABEL: bextr64c:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    movslq %esi, %rax
-; CHECK-NEXT:    bextrq %rax, %rdi, %rax
+; CHECK-NEXT:    # kill: def $esi killed $esi def $rsi
+; CHECK-NEXT:    bextrq %rsi, %rdi, %rax
 ; CHECK-NEXT:    retq
   %tmp0 = sext i32 %y to i64
   %tmp1 = tail call i64 @llvm.x86.bmi.bextr.64(i64 %x, i64 %tmp0)
   ret i64 %tmp1
 }
 
 define i64 @bextr64d(i64 %a) {
 ; CHECK-LABEL: bextr64d:

test/CodeGen/X86/bmi.ll

 ; X86-LABEL: bextr32c:
 ; X86:       # %bb.0:
 ; X86-NEXT:    movswl {{[0-9]+}}(%esp), %eax
 ; X86-NEXT:    bextrl %eax, {{[0-9]+}}(%esp), %eax
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: bextr32c:
 ; X64:       # %bb.0:
-; X64-NEXT:    movswl %si, %eax
-; X64-NEXT:    bextrl %eax, %edi, %eax
+; X64-NEXT:    bextrl %esi, %edi, %eax
 ; X64-NEXT:    retq
   %tmp0 = sext i16 %y to i32
   %tmp1 = tail call i32 @llvm.x86.bmi.bextr.32(i32 %x, i32 %tmp0)
   ret i32 %tmp1
 }
 
 define i32 @non_bextr32(i32 %x) {
 ; X86-LABEL: non_bextr32: