[RISCV] Return true in hasBitTest when Zbs is enabled and update BEXTI pattern for resulting canonicalisation

[asb]

Summary

As the Zbs extension includes bext[i] for bit extract, we can
unconditionally return true from this hook. This hook causes the DAG
combiner to perform the following canonicalisation:

and (not (srl X, C)), 1 --> (and X, 1<<C) == 0
and (srl (not X), C)), 1 --> (and X, 1<<C) == 0

As simply changing the hook causes a codegen regression, this patch also
modifies a BEXTI pattern to match this canonicalised form.

As BSETINVMask is now used for BEXT as well as BSET and BINV, it has
been renamed to the more generic SingleBitSetMask.

There is one codegen change in bittest.ll for bittest_31_i64 (NOT+BEXTI
rather than NOT+SRLIW). This is neutral in terms of code quality.

[reames]

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td

889

I think this needs to use BSETINVMask. If I follow the existing logic right, we're preferring and/xor for immediate masks which fit within 12 bits.

895–896

This pattern looks to be matching the old canonicalization, but oddly, not using the BINV.

[craig.topper]

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td

889

BINVI just toggles the bit in place. It doesn’t produce 0 or 1.

895–896

We can’t use binvi. It doesn’t return a 0 or 1 value.

[asb]

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td

895–896

Oops, that's a silly mistake on my part. I'll respin this patch.

[asb]

Fix error in first version of this patch, update patch description.

[reames]

LGTM

Glancing at the pattern, I find myself wondering if we can generalize further. You have:

def : Pat<(seteq (and GPR:$rs1, SingleBitSetMask:$mask), 0),
          (BEXTI (XORI GPR:$rs1, -1), SingleBitSetMask:$mask)>;

Could we do something like:

def : Pat<(and GPR:$rs1, SingleBitSetMask:$mask),
          (BEXTI GPR:$rs1, SingleBitSetMask:$mask)>;

Follow up only to be clear.

[craig.topper]

In D131482#3712522, @reames wrote:

LGTM

Glancing at the pattern, I find myself wondering if we can generalize further. You have:

def : Pat<(seteq (and GPR:$rs1, SingleBitSetMask:$mask), 0),
          (BEXTI (XORI GPR:$rs1, -1), SingleBitSetMask:$mask)>;

Could we do something like:

def : Pat<(and GPR:$rs1, SingleBitSetMask:$mask),
          (BEXTI GPR:$rs1, SingleBitSetMask:$mask)>;

Follow up only to be clear.

That does not work. BEXTI returns 0 or 1. An And keeps the bit in the same place.

[reames]

In D131482#3713009, @craig.topper wrote:

In D131482#3712522, @reames wrote:

LGTM

Glancing at the pattern, I find myself wondering if we can generalize further. You have:

def : Pat<(seteq (and GPR:$rs1, SingleBitSetMask:$mask), 0),
          (BEXTI (XORI GPR:$rs1, -1), SingleBitSetMask:$mask)>;

Could we do something like:

def : Pat<(and GPR:$rs1, SingleBitSetMask:$mask),
          (BEXTI GPR:$rs1, SingleBitSetMask:$mask)>;

Follow up only to be clear.

That does not work. BEXTI returns 0 or 1. An And keeps the bit in the same place.

Doh. And the same mistake I already made once in this review as well. Oops.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

if (VT.isVector())
     return false;
 
   return (Subtarget.hasStdExtZbb() || Subtarget.hasStdExtZbp() ||
           Subtarget.hasStdExtZbkb()) &&
          !isa<ConstantSDNode>(Y);
 }
 
 bool RISCVTargetLowering::hasBitTest(SDValue X, SDValue Y) const {
+  // Zbs provides BEXT[_I], which can be used with SEQZ/SNEZ as a bit test.
+  if (Subtarget.hasStdExtZbs())
+    return X.getValueType().isScalarInteger();
   // We can use ANDI+SEQZ/SNEZ as a bit test. Y contains the bit position.
   auto *C = dyn_cast<ConstantSDNode>(Y);
   return C && C->getAPIntValue().ule(10);
 }
 
 bool RISCVTargetLowering::shouldConvertConstantLoadToIntImm(const APInt &Imm,
                                                             Type *Ty) const {
   assert(Ty->isIntegerTy());

llvm/lib/Target/RISCV/RISCVInstrInfoZb.td

 }
 
 def BCLRXForm : SDNodeXForm<imm, [{
   // Find the lowest 0.
   return CurDAG->getTargetConstant(countTrailingOnes(N->getZExtValue()),
                                    SDLoc(N), N->getValueType(0));
 }]>;
 
-def BSETINVXForm : SDNodeXForm<imm, [{
+def SingleBitSetMaskToIndex : SDNodeXForm<imm, [{
   // Find the lowest 1.
   return CurDAG->getTargetConstant(countTrailingZeros(N->getZExtValue()),
                                    SDLoc(N), N->getValueType(0));
 }]>;
 
 // Checks if this mask has a single 0 bit and cannot be used with ANDI.
 def BCLRMask : ImmLeaf<XLenVT, [{
   if (Subtarget->is64Bit())
     return !isInt<12>(Imm) && isPowerOf2_64(~Imm);
   return !isInt<12>(Imm) && isPowerOf2_32(~Imm);
 }], BCLRXForm>;
 
 // Checks if this mask has a single 1 bit and cannot be used with ORI/XORI.
-def BSETINVMask : ImmLeaf<XLenVT, [{
+def SingleBitSetMask : ImmLeaf<XLenVT, [{
   if (Subtarget->is64Bit())
     return !isInt<12>(Imm) && isPowerOf2_64(Imm);
   return !isInt<12>(Imm) && isPowerOf2_32(Imm);
-}], BSETINVXForm>;
+}], SingleBitSetMaskToIndex>;
 
 // Check if (or r, i) can be optimized to (BSETI (BSETI r, i0), i1),
 // in which i = (1 << i0) | (1 << i1).
 def BSETINVTwoBitsMask : PatLeaf<(imm), [{
   if (!N->hasOneUse())
     return false;
   // The immediate should not be a simm12.
   if (isInt<12>(N->getSExtValue()))

 def : Pat<(and (shiftop<srl> GPR:$rs1, GPR:$rs2), 1),
           (BEXT GPR:$rs1, GPR:$rs2)>;
 
 def : Pat<(shiftop<shl> 1, GPR:$rs2),
           (BSET X0, GPR:$rs2)>;
 
 def : Pat<(and GPR:$rs1, BCLRMask:$mask),
           (BCLRI GPR:$rs1, BCLRMask:$mask)>;
-def : Pat<(or GPR:$rs1, BSETINVMask:$mask),
-          (BSETI GPR:$rs1, BSETINVMask:$mask)>;
-def : Pat<(xor GPR:$rs1, BSETINVMask:$mask),
-          (BINVI GPR:$rs1, BSETINVMask:$mask)>;
+def : Pat<(or GPR:$rs1, SingleBitSetMask:$mask),
+          (BSETI GPR:$rs1, SingleBitSetMask:$mask)>;
+def : Pat<(xor GPR:$rs1, SingleBitSetMask:$mask),
+          (BINVI GPR:$rs1, SingleBitSetMask:$mask)>;
 
 def : Pat<(and (srl GPR:$rs1, uimmlog2xlen:$shamt), (XLenVT 1)),
           (BEXTI GPR:$rs1, uimmlog2xlen:$shamt)>;
 
-def : Pat<(and (not (srl GPR:$rs1, uimmlog2xlen:$shamt)), (XLenVT 1)),
-          (XORI (BEXTI GPR:$rs1, uimmlog2xlen:$shamt), (XLenVT 1))>;
+def : Pat<(seteq (and GPR:$rs1, SingleBitSetMask:$mask), 0),
+          (BEXTI (XORI GPR:$rs1, -1), SingleBitSetMask:$mask)>;
 
 def : Pat<(or GPR:$r, BSETINVTwoBitsMask:$i),
           (BSETI (BSETI GPR:$r, (TrailingZerosXForm BSETINVTwoBitsMask:$i)),
                  (BSETINVTwoBitsMaskHigh BSETINVTwoBitsMask:$i))>;
 def : Pat<(xor GPR:$r, BSETINVTwoBitsMask:$i),
           (BINVI (BINVI GPR:$r, (TrailingZerosXForm BSETINVTwoBitsMask:$i)),
                  (BSETINVTwoBitsMaskHigh BSETINVTwoBitsMask:$i))>;
 def : Pat<(or GPR:$r, BSETINVORIMask:$i),

llvm/test/CodeGen/RISCV/bittest.ll

 define i64 @bittest_31_i64(i64 %a) nounwind {
 ; RV32-LABEL: bittest_31_i64:
 ; RV32:       # %bb.0:
 ; RV32-NEXT:    not a0, a0
 ; RV32-NEXT:    srli a0, a0, 31
 ; RV32-NEXT:    li a1, 0
 ; RV32-NEXT:    ret
 ;
-; RV64-LABEL: bittest_31_i64:
-; RV64:       # %bb.0:
-; RV64-NEXT:    not a0, a0
-; RV64-NEXT:    srliw a0, a0, 31
-; RV64-NEXT:    ret
+; RV64I-LABEL: bittest_31_i64:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    not a0, a0
+; RV64I-NEXT:    srliw a0, a0, 31
+; RV64I-NEXT:    ret
+;
+; RV64ZBS-LABEL: bittest_31_i64:
+; RV64ZBS:       # %bb.0:
+; RV64ZBS-NEXT:    not a0, a0
+; RV64ZBS-NEXT:    bexti a0, a0, 31
+; RV64ZBS-NEXT:    ret
   %shr = lshr i64 %a, 31
   %not = xor i64 %shr, -1
   %and = and i64 %not, 1
   ret i64 %and
 }
 
 define i64 @bittest_32_i64(i64 %a) nounwind {
 ; RV32-LABEL: bittest_32_i64: