Skip to content

[GISel] Fold bitreverse(shl/srl(bitreverse(x),y)) -> srl/shl(x,y) #91355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 8, 2024
Merged

[GISel] Fold bitreverse(shl/srl(bitreverse(x),y)) -> srl/shl(x,y) #91355

merged 1 commit into from
May 8, 2024

Conversation

RKSimon
Copy link
Collaborator

@RKSimon RKSimon commented May 7, 2024

Sibling patch to #89897

@RKSimon RKSimon requested a review from arsenm May 7, 2024 16:10
@llvmbot
Copy link
Member

llvmbot commented May 7, 2024

@llvm/pr-subscribers-llvm-globalisel

Author: Simon Pilgrim (RKSimon)

Changes

Sibling patch to #89897

Full diff: https://github.com/llvm/llvm-project/pull/91355.diff

5 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h (+3)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h (+6)
  • (modified) llvm/include/llvm/Target/GlobalISel/Combine.td (+8-1)
  • (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+30)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll (+34-98)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
index 4f1c9642e117..82f5e340d372 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
@@ -320,6 +320,9 @@ class CombinerHelper {
   void applyCombineShlOfExtend(MachineInstr &MI,
                                const RegisterImmPair &MatchData);
 
+  /// Fold bitreverse(shift (bitreverse x), y)) -> (shift x, y)
+  bool matchBitreverseShift(MachineInstr &MI, BuildFnTy &MatchInfo);
+
   /// Fold away a merge of an unmerge of the corresponding values.
   bool matchCombineMergeUnmerge(MachineInstr &MI, Register &MatchInfo);
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h b/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
index ea6ed322e9b1..af220e7cb652 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
@@ -598,6 +598,12 @@ m_GBitcast(const SrcTy &Src) {
   return UnaryOp_match<SrcTy, TargetOpcode::G_BITCAST>(Src);
 }
 
+template <typename SrcTy>
+inline UnaryOp_match<SrcTy, TargetOpcode::G_BITREVERSE>
+m_GBitreverse(const SrcTy &Src) {
+  return UnaryOp_match<SrcTy, TargetOpcode::G_BITREVERSE>(Src);
+}
+
 template <typename SrcTy>
 inline UnaryOp_match<SrcTy, TargetOpcode::G_PTRTOINT>
 m_GPtrToInt(const SrcTy &Src) {
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 72c5de03f4e7..011b405276f5 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -323,6 +323,13 @@ def reduce_shl_of_extend : GICombineRule<
          [{ return Helper.matchCombineShlOfExtend(*${mi}, ${matchinfo}); }]),
   (apply [{ Helper.applyCombineShlOfExtend(*${mi}, ${matchinfo}); }])>;
 
+// Combine bitreverse(shift (bitreverse x), y)) -> (shift x, y)
+def bitreverse_shift : GICombineRule<
+  (defs root:$d, build_fn_matchinfo:$matchinfo),
+  (match (wip_match_opcode G_BITREVERSE):$d,
+         [{ return Helper.matchBitreverseShift(*${d}, ${matchinfo}); }]),
+  (apply [{ Helper.applyBuildFn(*${d}, ${matchinfo}); }])>;
+
 // Combine (shl (add x, c1), c2) -> (add (shl x, c2), c1 << c2)
 // Combine (shl (or x, c1), c2) -> (or (shl x, c2), c1 << c2)
 def commute_shift : GICombineRule<
@@ -1658,7 +1665,7 @@ def all_combines : GICombineGroup<[trivial_combines, vector_ops_combines,
     unmerge_zext_to_zext, merge_unmerge, trunc_ext_fold, trunc_shift,
     const_combines, xor_of_and_with_same_reg, ptr_add_with_zero,
     shift_immed_chain, shift_of_shifted_logic_chain, load_or_combine,
-    div_rem_to_divrem, funnel_shift_combines, commute_shift,
+    div_rem_to_divrem, funnel_shift_combines, bitreverse_shift, commute_shift,
     form_bitfield_extract, constant_fold_binops, constant_fold_fma,
     constant_fold_cast_op, fabs_fneg_fold,
     intdiv_combines, mulh_combines, redundant_neg_operands,
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 653e7689b577..986edc4b28a9 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2016,6 +2016,36 @@ void CombinerHelper::applyCombineShlOfExtend(MachineInstr &MI,
   MI.eraseFromParent();
 }
 
+bool CombinerHelper::matchBitreverseShift(MachineInstr &MI,
+                                          BuildFnTy &MatchInfo) {
+  assert(MI.getOpcode() == TargetOpcode::G_BITREVERSE && "Expected BITREVERSE");
+  Register Dst = MI.getOperand(0).getReg();
+  Register Src = MI.getOperand(1).getReg();
+  Register Val, Amt;
+
+  // fold (bitreverse (shl (bitreverse x), y)) -> (lshr x, y)
+  if (mi_match(Src, MRI, m_GShl(m_GBitreverse(m_Reg(Val)), m_Reg(Amt))) &&
+      isLegalOrBeforeLegalizer(
+          {TargetOpcode::G_LSHR, {MRI.getType(Val), MRI.getType(Amt)}})) {
+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.buildInstr(TargetOpcode::G_LSHR, {Dst}, {Val, Amt});
+    };
+    return true;
+  }
+
+  // fold (bitreverse (lshr (bitreverse x), y)) -> (shl x, y)
+  if (mi_match(Src, MRI, m_GLShr(m_GBitreverse(m_Reg(Val)), m_Reg(Amt))) &&
+      isLegalOrBeforeLegalizer(
+          {TargetOpcode::G_SHL, {MRI.getType(Val), MRI.getType(Amt)}})) {
+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.buildInstr(TargetOpcode::G_SHL, {Dst}, {Val, Amt});
+    };
+    return true;
+  }
+
+  return false;
+}
+
 bool CombinerHelper::matchCombineMergeUnmerge(MachineInstr &MI,
                                               Register &MatchInfo) {
   GMerge &Merge = cast<GMerge>(MI);
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll b/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll
index 3ce94e2c40a9..b9fbe2379a42 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s --check-prefixes=SDAG
-; RUN: llc < %s -mtriple=aarch64-unknown-unknown -global-isel | FileCheck %s --check-prefixes=GISEL
+; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s
+; RUN: llc < %s -mtriple=aarch64-unknown-unknown -global-isel | FileCheck %s
 
 ; These tests can be optimised
 ;       fold (bitreverse(srl (bitreverse c), x)) -> (shl c, x)
@@ -12,19 +12,10 @@ declare i32 @llvm.bitreverse.i32(i32)
 declare i64 @llvm.bitreverse.i64(i64)
 
 define i8 @test_bitreverse_srli_bitreverse_i8(i8 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i8:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl w0, w0, #3
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i8:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #24
-; GISEL-NEXT:    lsr w8, w8, #3
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #24
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i8:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w0, w0, #3
+; CHECK-NEXT:    ret
   %1 = call i8 @llvm.bitreverse.i8(i8 %a)
   %2 = lshr i8 %1, 3
   %3 = call i8 @llvm.bitreverse.i8(i8 %2)
@@ -32,19 +23,10 @@ define i8 @test_bitreverse_srli_bitreverse_i8(i8 %a) nounwind {
 }
 
 define i16 @test_bitreverse_srli_bitreverse_i16(i16 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i16:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl w0, w0, #7
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i16:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #16
-; GISEL-NEXT:    lsr w8, w8, #7
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #16
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i16:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w0, w0, #7
+; CHECK-NEXT:    ret
   %1 = call i16 @llvm.bitreverse.i16(i16 %a)
   %2 = lshr i16 %1, 7
   %3 = call i16 @llvm.bitreverse.i16(i16 %2)
@@ -52,17 +34,10 @@ define i16 @test_bitreverse_srli_bitreverse_i16(i16 %a) nounwind {
 }
 
 define i32 @test_bitreverse_srli_bitreverse_i32(i32 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i32:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl w0, w0, #15
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i32:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #15
-; GISEL-NEXT:    rbit w0, w8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w0, w0, #15
+; CHECK-NEXT:    ret
   %1 = call i32 @llvm.bitreverse.i32(i32 %a)
   %2 = lshr i32 %1, 15
   %3 = call i32 @llvm.bitreverse.i32(i32 %2)
@@ -70,17 +45,10 @@ define i32 @test_bitreverse_srli_bitreverse_i32(i32 %a) nounwind {
 }
 
 define i64 @test_bitreverse_srli_bitreverse_i64(i64 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i64:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl x0, x0, #33
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i64:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit x8, x0
-; GISEL-NEXT:    lsr x8, x8, #33
-; GISEL-NEXT:    rbit x0, x8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i64:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl x0, x0, #33
+; CHECK-NEXT:    ret
   %1 = call i64 @llvm.bitreverse.i64(i64 %a)
   %2 = lshr i64 %1, 33
   %3 = call i64 @llvm.bitreverse.i64(i64 %2)
@@ -88,19 +56,10 @@ define i64 @test_bitreverse_srli_bitreverse_i64(i64 %a) nounwind {
 }
 
 define i8 @test_bitreverse_shli_bitreverse_i8(i8 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i8:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    ubfx w0, w0, #3, #5
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i8:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #24
-; GISEL-NEXT:    lsl w8, w8, #3
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #24
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i8:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ubfx w0, w0, #3, #5
+; CHECK-NEXT:    ret
   %1 = call i8 @llvm.bitreverse.i8(i8 %a)
   %2 = shl i8 %1, 3
   %3 = call i8 @llvm.bitreverse.i8(i8 %2)
@@ -108,19 +67,10 @@ define i8 @test_bitreverse_shli_bitreverse_i8(i8 %a) nounwind {
 }
 
 define i16 @test_bitreverse_shli_bitreverse_i16(i16 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i16:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    ubfx w0, w0, #7, #9
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i16:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #16
-; GISEL-NEXT:    lsl w8, w8, #7
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #16
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i16:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ubfx w0, w0, #7, #9
+; CHECK-NEXT:    ret
   %1 = call i16 @llvm.bitreverse.i16(i16 %a)
   %2 = shl i16 %1, 7
   %3 = call i16 @llvm.bitreverse.i16(i16 %2)
@@ -128,17 +78,10 @@ define i16 @test_bitreverse_shli_bitreverse_i16(i16 %a) nounwind {
 }
 
 define i32 @test_bitreverse_shli_bitreverse_i32(i32 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i32:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsr w0, w0, #15
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i32:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsl w8, w8, #15
-; GISEL-NEXT:    rbit w0, w8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsr w0, w0, #15
+; CHECK-NEXT:    ret
   %1 = call i32 @llvm.bitreverse.i32(i32 %a)
   %2 = shl i32 %1, 15
   %3 = call i32 @llvm.bitreverse.i32(i32 %2)
@@ -146,17 +89,10 @@ define i32 @test_bitreverse_shli_bitreverse_i32(i32 %a) nounwind {
 }
 
 define i64 @test_bitreverse_shli_bitreverse_i64(i64 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i64:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsr x0, x0, #33
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i64:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit x8, x0
-; GISEL-NEXT:    lsl x8, x8, #33
-; GISEL-NEXT:    rbit x0, x8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i64:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsr x0, x0, #33
+; CHECK-NEXT:    ret
   %1 = call i64 @llvm.bitreverse.i64(i64 %a)
   %2 = shl i64 %1, 33
   %3 = call i64 @llvm.bitreverse.i64(i64 %2)

@llvmbot
Copy link
Member

llvmbot commented May 7, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Simon Pilgrim (RKSimon)

Changes

Sibling patch to #89897

Full diff: https://github.com/llvm/llvm-project/pull/91355.diff

5 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h (+3)
  • (modified) llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h (+6)
  • (modified) llvm/include/llvm/Target/GlobalISel/Combine.td (+8-1)
  • (modified) llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp (+30)
  • (modified) llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll (+34-98)
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
index 4f1c9642e117..82f5e340d372 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
@@ -320,6 +320,9 @@ class CombinerHelper {
   void applyCombineShlOfExtend(MachineInstr &MI,
                                const RegisterImmPair &MatchData);
 
+  /// Fold bitreverse(shift (bitreverse x), y)) -> (shift x, y)
+  bool matchBitreverseShift(MachineInstr &MI, BuildFnTy &MatchInfo);
+
   /// Fold away a merge of an unmerge of the corresponding values.
   bool matchCombineMergeUnmerge(MachineInstr &MI, Register &MatchInfo);
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h b/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
index ea6ed322e9b1..af220e7cb652 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
@@ -598,6 +598,12 @@ m_GBitcast(const SrcTy &Src) {
   return UnaryOp_match<SrcTy, TargetOpcode::G_BITCAST>(Src);
 }
 
+template <typename SrcTy>
+inline UnaryOp_match<SrcTy, TargetOpcode::G_BITREVERSE>
+m_GBitreverse(const SrcTy &Src) {
+  return UnaryOp_match<SrcTy, TargetOpcode::G_BITREVERSE>(Src);
+}
+
 template <typename SrcTy>
 inline UnaryOp_match<SrcTy, TargetOpcode::G_PTRTOINT>
 m_GPtrToInt(const SrcTy &Src) {
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 72c5de03f4e7..011b405276f5 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -323,6 +323,13 @@ def reduce_shl_of_extend : GICombineRule<
          [{ return Helper.matchCombineShlOfExtend(*${mi}, ${matchinfo}); }]),
   (apply [{ Helper.applyCombineShlOfExtend(*${mi}, ${matchinfo}); }])>;
 
+// Combine bitreverse(shift (bitreverse x), y)) -> (shift x, y)
+def bitreverse_shift : GICombineRule<
+  (defs root:$d, build_fn_matchinfo:$matchinfo),
+  (match (wip_match_opcode G_BITREVERSE):$d,
+         [{ return Helper.matchBitreverseShift(*${d}, ${matchinfo}); }]),
+  (apply [{ Helper.applyBuildFn(*${d}, ${matchinfo}); }])>;
+
 // Combine (shl (add x, c1), c2) -> (add (shl x, c2), c1 << c2)
 // Combine (shl (or x, c1), c2) -> (or (shl x, c2), c1 << c2)
 def commute_shift : GICombineRule<
@@ -1658,7 +1665,7 @@ def all_combines : GICombineGroup<[trivial_combines, vector_ops_combines,
     unmerge_zext_to_zext, merge_unmerge, trunc_ext_fold, trunc_shift,
     const_combines, xor_of_and_with_same_reg, ptr_add_with_zero,
     shift_immed_chain, shift_of_shifted_logic_chain, load_or_combine,
-    div_rem_to_divrem, funnel_shift_combines, commute_shift,
+    div_rem_to_divrem, funnel_shift_combines, bitreverse_shift, commute_shift,
     form_bitfield_extract, constant_fold_binops, constant_fold_fma,
     constant_fold_cast_op, fabs_fneg_fold,
     intdiv_combines, mulh_combines, redundant_neg_operands,
diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 653e7689b577..986edc4b28a9 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -2016,6 +2016,36 @@ void CombinerHelper::applyCombineShlOfExtend(MachineInstr &MI,
   MI.eraseFromParent();
 }
 
+bool CombinerHelper::matchBitreverseShift(MachineInstr &MI,
+                                          BuildFnTy &MatchInfo) {
+  assert(MI.getOpcode() == TargetOpcode::G_BITREVERSE && "Expected BITREVERSE");
+  Register Dst = MI.getOperand(0).getReg();
+  Register Src = MI.getOperand(1).getReg();
+  Register Val, Amt;
+
+  // fold (bitreverse (shl (bitreverse x), y)) -> (lshr x, y)
+  if (mi_match(Src, MRI, m_GShl(m_GBitreverse(m_Reg(Val)), m_Reg(Amt))) &&
+      isLegalOrBeforeLegalizer(
+          {TargetOpcode::G_LSHR, {MRI.getType(Val), MRI.getType(Amt)}})) {
+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.buildInstr(TargetOpcode::G_LSHR, {Dst}, {Val, Amt});
+    };
+    return true;
+  }
+
+  // fold (bitreverse (lshr (bitreverse x), y)) -> (shl x, y)
+  if (mi_match(Src, MRI, m_GLShr(m_GBitreverse(m_Reg(Val)), m_Reg(Amt))) &&
+      isLegalOrBeforeLegalizer(
+          {TargetOpcode::G_SHL, {MRI.getType(Val), MRI.getType(Amt)}})) {
+    MatchInfo = [=](MachineIRBuilder &B) {
+      B.buildInstr(TargetOpcode::G_SHL, {Dst}, {Val, Amt});
+    };
+    return true;
+  }
+
+  return false;
+}
+
 bool CombinerHelper::matchCombineMergeUnmerge(MachineInstr &MI,
                                               Register &MatchInfo) {
   GMerge &Merge = cast<GMerge>(MI);
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll b/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll
index 3ce94e2c40a9..b9fbe2379a42 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/combine-bitreverse-shift.ll
@@ -1,6 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s --check-prefixes=SDAG
-; RUN: llc < %s -mtriple=aarch64-unknown-unknown -global-isel | FileCheck %s --check-prefixes=GISEL
+; RUN: llc < %s -mtriple=aarch64-unknown-unknown | FileCheck %s
+; RUN: llc < %s -mtriple=aarch64-unknown-unknown -global-isel | FileCheck %s
 
 ; These tests can be optimised
 ;       fold (bitreverse(srl (bitreverse c), x)) -> (shl c, x)
@@ -12,19 +12,10 @@ declare i32 @llvm.bitreverse.i32(i32)
 declare i64 @llvm.bitreverse.i64(i64)
 
 define i8 @test_bitreverse_srli_bitreverse_i8(i8 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i8:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl w0, w0, #3
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i8:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #24
-; GISEL-NEXT:    lsr w8, w8, #3
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #24
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i8:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w0, w0, #3
+; CHECK-NEXT:    ret
   %1 = call i8 @llvm.bitreverse.i8(i8 %a)
   %2 = lshr i8 %1, 3
   %3 = call i8 @llvm.bitreverse.i8(i8 %2)
@@ -32,19 +23,10 @@ define i8 @test_bitreverse_srli_bitreverse_i8(i8 %a) nounwind {
 }
 
 define i16 @test_bitreverse_srli_bitreverse_i16(i16 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i16:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl w0, w0, #7
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i16:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #16
-; GISEL-NEXT:    lsr w8, w8, #7
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #16
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i16:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w0, w0, #7
+; CHECK-NEXT:    ret
   %1 = call i16 @llvm.bitreverse.i16(i16 %a)
   %2 = lshr i16 %1, 7
   %3 = call i16 @llvm.bitreverse.i16(i16 %2)
@@ -52,17 +34,10 @@ define i16 @test_bitreverse_srli_bitreverse_i16(i16 %a) nounwind {
 }
 
 define i32 @test_bitreverse_srli_bitreverse_i32(i32 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i32:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl w0, w0, #15
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i32:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #15
-; GISEL-NEXT:    rbit w0, w8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl w0, w0, #15
+; CHECK-NEXT:    ret
   %1 = call i32 @llvm.bitreverse.i32(i32 %a)
   %2 = lshr i32 %1, 15
   %3 = call i32 @llvm.bitreverse.i32(i32 %2)
@@ -70,17 +45,10 @@ define i32 @test_bitreverse_srli_bitreverse_i32(i32 %a) nounwind {
 }
 
 define i64 @test_bitreverse_srli_bitreverse_i64(i64 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_srli_bitreverse_i64:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsl x0, x0, #33
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_srli_bitreverse_i64:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit x8, x0
-; GISEL-NEXT:    lsr x8, x8, #33
-; GISEL-NEXT:    rbit x0, x8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_srli_bitreverse_i64:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsl x0, x0, #33
+; CHECK-NEXT:    ret
   %1 = call i64 @llvm.bitreverse.i64(i64 %a)
   %2 = lshr i64 %1, 33
   %3 = call i64 @llvm.bitreverse.i64(i64 %2)
@@ -88,19 +56,10 @@ define i64 @test_bitreverse_srli_bitreverse_i64(i64 %a) nounwind {
 }
 
 define i8 @test_bitreverse_shli_bitreverse_i8(i8 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i8:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    ubfx w0, w0, #3, #5
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i8:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #24
-; GISEL-NEXT:    lsl w8, w8, #3
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #24
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i8:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ubfx w0, w0, #3, #5
+; CHECK-NEXT:    ret
   %1 = call i8 @llvm.bitreverse.i8(i8 %a)
   %2 = shl i8 %1, 3
   %3 = call i8 @llvm.bitreverse.i8(i8 %2)
@@ -108,19 +67,10 @@ define i8 @test_bitreverse_shli_bitreverse_i8(i8 %a) nounwind {
 }
 
 define i16 @test_bitreverse_shli_bitreverse_i16(i16 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i16:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    ubfx w0, w0, #7, #9
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i16:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsr w8, w8, #16
-; GISEL-NEXT:    lsl w8, w8, #7
-; GISEL-NEXT:    rbit w8, w8
-; GISEL-NEXT:    lsr w0, w8, #16
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i16:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ubfx w0, w0, #7, #9
+; CHECK-NEXT:    ret
   %1 = call i16 @llvm.bitreverse.i16(i16 %a)
   %2 = shl i16 %1, 7
   %3 = call i16 @llvm.bitreverse.i16(i16 %2)
@@ -128,17 +78,10 @@ define i16 @test_bitreverse_shli_bitreverse_i16(i16 %a) nounwind {
 }
 
 define i32 @test_bitreverse_shli_bitreverse_i32(i32 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i32:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsr w0, w0, #15
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i32:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit w8, w0
-; GISEL-NEXT:    lsl w8, w8, #15
-; GISEL-NEXT:    rbit w0, w8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i32:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsr w0, w0, #15
+; CHECK-NEXT:    ret
   %1 = call i32 @llvm.bitreverse.i32(i32 %a)
   %2 = shl i32 %1, 15
   %3 = call i32 @llvm.bitreverse.i32(i32 %2)
@@ -146,17 +89,10 @@ define i32 @test_bitreverse_shli_bitreverse_i32(i32 %a) nounwind {
 }
 
 define i64 @test_bitreverse_shli_bitreverse_i64(i64 %a) nounwind {
-; SDAG-LABEL: test_bitreverse_shli_bitreverse_i64:
-; SDAG:       // %bb.0:
-; SDAG-NEXT:    lsr x0, x0, #33
-; SDAG-NEXT:    ret
-;
-; GISEL-LABEL: test_bitreverse_shli_bitreverse_i64:
-; GISEL:       // %bb.0:
-; GISEL-NEXT:    rbit x8, x0
-; GISEL-NEXT:    lsl x8, x8, #33
-; GISEL-NEXT:    rbit x0, x8
-; GISEL-NEXT:    ret
+; CHECK-LABEL: test_bitreverse_shli_bitreverse_i64:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    lsr x0, x0, #33
+; CHECK-NEXT:    ret
   %1 = call i64 @llvm.bitreverse.i64(i64 %a)
   %2 = shl i64 %1, 33
   %3 = call i64 @llvm.bitreverse.i64(i64 %2)

@tschuett
Copy link

tschuett commented May 7, 2024 •
edited
Loading

https://llvm.org/docs/GlobalISel/MIRPatterns.html#gallery

(match (G_SHL $src, $x, $y), (G_BITREVERSE $root, $src)), [{ Helper.match .. }],


(match (G_BITREVERSE $bit, $x),
       (G_SHL $src, $bit, $y),
       (G_BITREVERSE $root, $src)

@tschuett tschuett self-requested a review May 7, 2024 16:30
tschuett
tschuett reviewed May 7, 2024
View reviewed changes
arsenm
arsenm reviewed May 7, 2024
View reviewed changes
Copy link
Contributor

@arsenm arsenm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to add something for legality rules in patterns

@RKSimon RKSimon force-pushed the gisel-bitreverse-shift branch 2 times, most recently from bf7343a to 049eefd Compare May 7, 2024 17:50
RKSimon
RKSimon commented May 7, 2024
View reviewed changes
llvm/include/llvm/Target/GlobalISel/Combine.td Outdated
(G_LSHR $src, $rev, $amt),
(G_BITREVERSE $d, $src):$mi,
[{ return Helper.matchBitreverseShift(*${mi}, ${matchinfo}); }]),
(apply [{ Helper.applyBuildFn(*${mi}, ${matchinfo}); }])>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had to keep the matchBitreverseShift for legality tests, but the full patterns are now tested in the td file.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. If you match for G_BITREVERSE, then the combiner will jump for every G_BITREVERSE into C++ and fail in most cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyway that we could replace the matchBitreverseShift call with a legality check?

arsenm
arsenm reviewed May 8, 2024
View reviewed changes
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp Outdated
Comment on lines 2026 to 2040
// fold (bitreverse (shl (bitreverse x), y)) -> (lshr x, y)
if (mi_match(Src, MRI, m_GShl(m_GBitreverse(m_Reg(Val)), m_Reg(Amt))) &&
isLegalOrBeforeLegalizer(
{TargetOpcode::G_LSHR, {MRI.getType(Val), MRI.getType(Amt)}})) {
MatchInfo = [=](MachineIRBuilder &B) { B.buildLShr(Dst, Val, Amt); };
return true;
}

// fold (bitreverse (lshr (bitreverse x), y)) -> (shl x, y)
if (mi_match(Src, MRI, m_GLShr(m_GBitreverse(m_Reg(Val)), m_Reg(Amt))) &&
isLegalOrBeforeLegalizer(
{TargetOpcode::G_SHL, {MRI.getType(Val), MRI.getType(Amt)}})) {
MatchInfo = [=](MachineIRBuilder &B) { B.buildShl(Dst, Val, Amt); };
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate you have to rematch the same pattern in here. Should split these into separate apply functions, the 2 entry points just hit the 2 different cases here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. I think you have to match them again because you are combining the "match" step in a single function. Splitting would just call the function - matchBitreverseShiftLeft/right and then just check for other conditions.

@arsenm arsenm requested a review from Pierre-vh May 8, 2024 07:40
tschuett
tschuett reviewed May 8, 2024
View reviewed changes
llvm/include/llvm/Target/GlobalISel/Combine.td Outdated
(match (G_BITREVERSE $rev, $val),
(G_LSHR $src, $rev, $amt),
(G_BITREVERSE $d, $src):$mi,
[{ return Helper.matchBitreverseShift(*${mi}, ${matchinfo}); }]),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing in a MachineInstr and reconstructing the pattern, you could pass in MachineOperands, e.g, d, val, amt. Then you can take the registers of the MOs, do the legality check, and build the shift.

@RKSimon RKSimon force-pushed the gisel-bitreverse-shift branch from 049eefd to 9da3544 Compare May 8, 2024 17:19
@RKSimon
Copy link
Collaborator Author

RKSimon commented May 8, 2024

@tschuett @arsenm I've managed to move all the legality checking into the TD patterns.

@RKSimon RKSimon merged commit 965f3ca into llvm:main May 8, 2024
@RKSimon RKSimon deleted the gisel-bitreverse-shift branch May 8, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madhur13490 madhur13490 madhur13490 left review comments

@tschuett tschuett tschuett left review comments

@arsenm arsenm arsenm approved these changes

@Pierre-vh Pierre-vh Awaiting requested review from Pierre-vh

Assignees
No one assigned
Labels
backend:AArch64 llvm:globalisel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RKSimon @llvmbot @tschuett @arsenm @madhur13490