[AArch64] Add support for lowering the bitreverse intrinsic to the rbit instruction.

[mcrosier]

Summary

This addresses PR31519. Clang patch: D28400.

PTAL,
Chad

[gberry]

lib/Target/AArch64/AArch64InstrInfo.td

955

I think you can get rid of these patterns and instead just change the defm RBIT above to pass the bitreverse fragment as the 3rd argument (as is done for CLZ).

[RKSimon]

lib/Target/AArch64/AArch64InstrInfo.td

954

Are these target intrinsics still necessary?

[mcrosier]

Address Simon and Geoff's feedback.

[RKSimon]

Add LowerINTRINSIC_WO_CHAIN or AutoUpgrade support to handle int_aarch64_rbit with bitreverse? See ARMTargetLowering::LowerINTRINSIC_WO_CHAIN

[mcrosier]

lib/Target/AArch64/AArch64InstrInfo.td

954

Good point, Simon. I believe this patch and D28400 together can allow us to remove the target-specific intrinsic.

[mcrosier]

Add support for lowering aarch64_rbit to BITREVERSE DAG node, per Simon's comment. Thanks, Simon.

[RKSimon]

include/llvm/IR/IntrinsicsAArch64.td

45

You can remove this right now if you add support in AutoUpgrade.cpp

[mcrosier]

-Use autoupgrade to allow removal of aarch64 rbit intrinsic, per Simon's suggestion.

[mcrosier]

Update a comment now that we're auto-upgrading.

[RKSimon]

No further comments from me.

[mcrosier]

In D28379#639792, @RKSimon wrote:

No further comments from me.

Thanks, Simon.

[mcrosier]

Ping (AArch64 contributors). This patch is in great shape thanks to Simon, but I assume he's deferring the official LGTM to a more frequent AArch64 contributor.

[sbaranga]

LGTM!

[mcrosier]

In D28379#641409, @sbaranga wrote:

LGTM!

Thanks, Silviu! Would you mind taking a quick look at the clang side patch: D28400? Very straight forward.

include/llvm/IR/IntrinsicsAArch64.td

 
 //===----------------------------------------------------------------------===//
 // HINT
 
 def int_aarch64_hint : Intrinsic<[], [llvm_i32_ty]>;
 
 //===----------------------------------------------------------------------===//
 // RBIT
-
+//
+// Note: We no longer generate this intrinsic in the front-end and instead
+// prefer the generic bitreverse intrinsic.  At some point we should remove
+// this intrinsic once we're willing to break bitcode compatibility.
 def int_aarch64_rbit : Intrinsic<[llvm_anyint_ty], [LLVMMatchType<0>],
                                  [IntrNoMem]>;
 
 //===----------------------------------------------------------------------===//
 // Data Barrier Instructions
 
 def int_aarch64_dmb : GCCBuiltin<"__builtin_arm_dmb">, Intrinsic<[], [llvm_i32_ty]>;
 def int_aarch64_dsb : GCCBuiltin<"__builtin_arm_dsb">, Intrinsic<[], [llvm_i32_ty]>;

lib/Target/AArch64/AArch64ISelLowering.cpp

AArch64TargetLowering::AArch64TargetLowering(const TargetMachine &TM,
 
   // Provide all sorts of operation actions
   setOperationAction(ISD::GlobalAddress, MVT::i64, Custom);
   setOperationAction(ISD::GlobalTLSAddress, MVT::i64, Custom);
   setOperationAction(ISD::SETCC, MVT::i32, Custom);
   setOperationAction(ISD::SETCC, MVT::i64, Custom);
   setOperationAction(ISD::SETCC, MVT::f32, Custom);
   setOperationAction(ISD::SETCC, MVT::f64, Custom);
+  setOperationAction(ISD::BITREVERSE, MVT::i32, Legal);
+  setOperationAction(ISD::BITREVERSE, MVT::i64, Legal);
   setOperationAction(ISD::BRCOND, MVT::Other, Expand);
   setOperationAction(ISD::BR_CC, MVT::i32, Custom);
   setOperationAction(ISD::BR_CC, MVT::i64, Custom);
   setOperationAction(ISD::BR_CC, MVT::f32, Custom);
   setOperationAction(ISD::BR_CC, MVT::f64, Custom);
   setOperationAction(ISD::SELECT, MVT::i32, Custom);
   setOperationAction(ISD::SELECT, MVT::i64, Custom);
   setOperationAction(ISD::SELECT, MVT::f32, Custom);

 }
 
 SDValue AArch64TargetLowering::LowerINTRINSIC_WO_CHAIN(SDValue Op,
                                                      SelectionDAG &DAG) const {
   unsigned IntNo = cast<ConstantSDNode>(Op.getOperand(0))->getZExtValue();
   SDLoc dl(Op);
   switch (IntNo) {
   default: return SDValue();    // Don't custom lower most intrinsics.
+  case Intrinsic::aarch64_rbit: {
+    EVT VT = Op.getOperand(1).getValueType();
+    assert((VT == MVT::i32 || VT == MVT::i64) &&
+           "RBIT intrinsic must have i32 or i64 type!");
+    return DAG.getNode(ISD::BITREVERSE, dl, VT, Op.getOperand(1));
+  }
   case Intrinsic::thread_pointer: {
     EVT PtrVT = getPointerTy(DAG.getDataLayout());
     return DAG.getNode(AArch64ISD::THREAD_POINTER, dl, PtrVT);
   }
   case Intrinsic::aarch64_neon_smax:
     return DAG.getNode(ISD::SMAX, dl, Op.getValueType(),
                        Op.getOperand(1), Op.getOperand(2));
   case Intrinsic::aarch64_neon_umax:

lib/Target/AArch64/AArch64InstrInfo.td

 
 
 //===----------------------------------------------------------------------===//
 // One operand data processing instructions.
 //===----------------------------------------------------------------------===//
 
 defm CLS    : OneOperandData<0b101, "cls">;
 defm CLZ    : OneOperandData<0b100, "clz", ctlz>;
-defm RBIT   : OneOperandData<0b000, "rbit">;
-
-def : Pat<(int_aarch64_rbit GPR32:$Rn), (RBITWr $Rn)>;
-def : Pat<(int_aarch64_rbit GPR64:$Rn), (RBITXr $Rn)>;
+defm RBIT   : OneOperandData<0b000, "rbit", bitreverse>;
 
 def  REV16Wr : OneWRegData<0b001, "rev16",
                                   UnOpFrag<(rotr (bswap node:$LHS), (i64 16))>>;
 def  REV16Xr : OneXRegData<0b001, "rev16", null_frag>;
 
 def : Pat<(cttz GPR32:$Rn),
           (CLZWr (RBITWr GPR32:$Rn))>;
 def : Pat<(cttz GPR64:$Rn),
           (CLZXr (RBITXr GPR64:$Rn))>;

test/CodeGen/AArch64/bitreverse.ll

 ; RUN: llc -mtriple=aarch64-eabi %s -o - | FileCheck %s
 
-; These tests just check that the plumbing is in place for @llvm.bitreverse. The
-; actual output is massive at the moment as llvm.bitreverse is not yet legal.
+; These tests just check that the plumbing is in place for @llvm.bitreverse.
 
 declare <2 x i16> @llvm.bitreverse.v2i16(<2 x i16>) readnone
 
 define <2 x i16> @f(<2 x i16> %a) {
 ; CHECK-LABEL: f:
-; CHECK: rev32
-; CHECK: ushr
+; CHECK: fmov [[REG1:w[0-9]+]], s0
+; CHECK-DAG: rbit [[REG2:w[0-9]+]], [[REG1]]
+; CHECK-DAG: fmov s0, [[REG2]]
+; CHECK-DAG: mov [[REG3:w[0-9]+]], v0.s[1]
+; CHECK-DAG: rbit [[REG4:w[0-9]+]], [[REG3]]
+; CHECK-DAG: ins v0.s[1], [[REG4]]
+; CHECK-DAG: ushr v0.2s, v0.2s, #16
   %b = call <2 x i16> @llvm.bitreverse.v2i16(<2 x i16> %a)
   ret <2 x i16> %b
 }
 
 declare i8 @llvm.bitreverse.i8(i8) readnone
 
 define i8 @g(i8 %a) {
 ; CHECK-LABEL: g:
-; CHECK-DAG: rev [[RV:w.*]], w0
-; CHECK-DAG: and [[L4:w.*]], [[RV]], #0xf0f0f0f
-; CHECK-DAG: and [[H4:w.*]], [[RV]], #0xf0f0f0f0
-; CHECK-DAG: lsr [[S4:w.*]], [[H4]], #4
-; CHECK-DAG: orr [[R4:w.*]], [[S4]], [[L4]], lsl #4
-
-; CHECK-DAG: and [[L2:w.*]], [[R4]], #0x33333333
-; CHECK-DAG: and [[H2:w.*]], [[R4]], #0xcccccccc
-; CHECK-DAG: lsr [[S2:w.*]], [[H2]], #2
-; CHECK-DAG: orr [[R2:w.*]], [[S2]], [[L2]], lsl #2
-
-; CHECK-DAG: mov [[P1:w.*]], #1426063360
-; CHECK-DAG: mov [[N1:w.*]], #-1442840576
-; CHECK-DAG: and [[L1:w.*]], [[R2]], [[P1]]
-; CHECK-DAG: and [[H1:w.*]], [[R2]], [[N1]]
-; CHECK-DAG: lsr [[S1:w.*]], [[H1]], #1
-; CHECK-DAG: orr [[R1:w.*]], [[S1]], [[L1]], lsl #1
-
-; CHECK-DAG: lsr w0, [[R1]], #24
-; CHECK-DAG: ret
+; CHECK: rbit [[REG:w[0-9]+]], w0
+; CHECK-NEXT: lsr w0, [[REG]], #24
+; CHECK-NEXT: ret
   %b = call i8 @llvm.bitreverse.i8(i8 %a)
   ret i8 %b
 }
 
 declare <8 x i8> @llvm.bitreverse.v8i8(<8 x i8>) readnone
 
 define <8 x i8> @g_vec(<8 x i8> %a) {
 ; CHECK-DAG: movi [[M1:v.*]], #15

test/CodeGen/AArch64/rbit.ll

 ; RUN: llc -mtriple=aarch64-eabi %s -o - | FileCheck %s
 
+;; The first two checks make sure we can lower the target-specific intrinsic to
+;; a BITREVERSE DAG node.
+
 ; CHECK-LABEL: rbit32
 ; CHECK: rbit w0, w0
 define i32 @rbit32(i32 %t) {
 entry:
   %rbit.i = call i32 @llvm.aarch64.rbit.i32(i32 %t)
   ret i32 %rbit.i
 }
 
 ; CHECK-LABEL: rbit64
 ; CHECK: rbit x0, x0
 define i64 @rbit64(i64 %t) {
 entry:
   %rbit.i = call i64 @llvm.aarch64.rbit.i64(i64 %t)
   ret i64 %rbit.i
 }
 
 declare i64 @llvm.aarch64.rbit.i64(i64)
 declare i32 @llvm.aarch64.rbit.i32(i32)
+
+; CHECK-LABEL: rbit_generic32
+; CHECK: rbit w0, w0
+define i32 @rbit_generic32(i32 %t) {
+entry:
+  %rbit = call i32 @llvm.bitreverse.i32(i32 %t)
+  ret i32 %rbit
+}
+
+; CHECK-LABEL: rbit_generic64
+; CHECK: rbit x0, x0
+define i64 @rbit_generic64(i64 %t) {
+entry:
+  %rbit = call i64 @llvm.bitreverse.i64(i64 %t)
+  ret i64 %rbit
+}
+
+declare i32 @llvm.bitreverse.i32(i32) readnone
+declare i64 @llvm.bitreverse.i64(i64) readnone