Skip to content

[RISCV] Remove artificial restriction on ShAmt from (shl (and X, C2), C) -> (srli (slli X, C4), C4-C) isel. #143010

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.

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

Merged
merged 2 commits into from
Jun 6, 2025

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jun 5, 2025

This code incorrectly inherited a ShAmt <= 32 check from an earlier pattern.

@llvmbot
Copy link
Member

llvmbot commented Jun 5, 2025

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

This code incorrectly inherited a ShAmt &lt;= 32 check from an earlier pattern.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp (+2-2)
  • (modified) llvm/test/CodeGen/RISCV/and-shl.ll (+18)
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index 4dd53fdd0213d..a5cdf9f79f57a 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -1051,11 +1051,11 @@ void RISCVDAGToDAGISel::Select(SDNode *Node) {
     unsigned ShAmt = N1C->getZExtValue();
     uint64_t Mask = N0.getConstantOperandVal(1);
 
-    if (ShAmt <= 32 && isShiftedMask_64(Mask)) {
+    if (isShiftedMask_64(Mask)) {
       unsigned XLen = Subtarget->getXLen();
       unsigned LeadingZeros = XLen - llvm::bit_width(Mask);
       unsigned TrailingZeros = llvm::countr_zero(Mask);
-      if (TrailingZeros > 0 && LeadingZeros == 32) {
+      if (ShAmt <= 32 && TrailingZeros > 0 && LeadingZeros == 32) {
         // Optimize (shl (and X, C2), C) -> (slli (srliw X, C3), C3+C)
         // where C2 has 32 leading zeros and C3 trailing zeros.
         SDNode *SRLIW = CurDAG->getMachineNode(
diff --git a/llvm/test/CodeGen/RISCV/and-shl.ll b/llvm/test/CodeGen/RISCV/and-shl.ll
index c3cb5d8e2e37d..26b4a90d88fba 100644
--- a/llvm/test/CodeGen/RISCV/and-shl.ll
+++ b/llvm/test/CodeGen/RISCV/and-shl.ll
@@ -77,3 +77,21 @@ define i32 @and_0xfff_shl_2_multi_use(i32 %x) {
   %r = add i32 %a, %s
   ret i32 %r
 }
+
+define i64 @and_0xfff_shl_33(i64 %x) {
+; RV32I-LABEL: and_0xfff_shl_33:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    slli a0, a0, 20
+; RV32I-NEXT:    srli a1, a0, 19
+; RV32I-NEXT:    li a0, 0
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: and_0xfff_shl_33:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    slli a0, a0, 52
+; RV64I-NEXT:    srli a0, a0, 19
+; RV64I-NEXT:    ret
+  %a = and i64 %x, 4095
+  %s = shl i64 %a, 33
+  ret i64 %s
+}

@pfusik
Copy link
Contributor

pfusik commented Jun 5, 2025

LGTM, modulo the commit message. I think you are relaxing the (shl (and X, C2), C) -> (srli (slli X, C4), C4-C) pattern.

@topperc
Copy link
Collaborator Author

topperc commented Jun 5, 2025

LGTM, modulo the commit message. I think you are relaxing the (shl (and X, C2), C) -> (srli (slli X, C4), C4-C) pattern.

You're right. I copied the wrong comment while I was writing the message

@topperc topperc changed the title [RISCV] Remove artificial restriction on ShAmt from (shl (and X, C2), C) -> (slli (srliw X, C3), C3+C) isel. [RISCV] Remove artificial restriction on ShAmt from (shl (and X, C2), C) -> (srli (slli X, C4), C4-C) isel. Jun 5, 2025
@topperc topperc merged commit a23bd17 into llvm:main Jun 6, 2025
13 checks passed
@topperc topperc deleted the pr/shamt branch June 6, 2025 00:48
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
… C) -> (srli (slli X, C4), C4-C) isel. (llvm#143010)

This code unnecessarily inherited a `ShAmt <= 32` check from an earlier
pattern.
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 2025
… C) -> (srli (slli X, C4), C4-C) isel. (llvm#143010)

This code unnecessarily inherited a `ShAmt <= 32` check from an earlier
pattern.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants