Skip to content
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

[ConstraintElim] Decompose shl nsw for signed predicates #76961

Merged
merged 2 commits into from
Jan 5, 2024

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jan 4, 2024

shl nsw x, shift can be interpreted as mul nsw x, (1<<shift), except when shift is bw-1 (https://alive2.llvm.org/ce/z/vDh2xT). Use this when decomposing shl. The equivalent decomposition for the unsigned case already exists.

shl nsw x, shift can be interpreted as mul nsw x, (1<<shift),
except when shift is bw-1 (https://alive2.llvm.org/ce/z/vDh2xT).
Use this when decomposing shl. The equivalent decomposition for
the unsigned case already exists.
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

shl nsw x, shift can be interpreted as mul nsw x, (1<<shift), except when shift is bw-1 (https://alive2.llvm.org/ce/z/vDh2xT). Use this when decomposing shl. The equivalent decomposition for the unsigned case already exists.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+11)
  • (modified) llvm/test/Transforms/ConstraintElimination/shl.ll (+3-6)
diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 9a814ba9fd7380..05c56cae4dacfb 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -517,6 +517,17 @@ static Decomposition decompose(Value *V,
       return Result;
     }
 
+    // shl nsw x, shift is mul nsw x, (1<<shift),
+    // with the exception of shift == bw-1.
+    if (match(V, m_NSWShl(m_Value(Op0), m_ConstantInt(CI)))) {
+      uint64_t Shift = CI->getValue().getLimitedValue();
+      if (Shift < Ty->getIntegerBitWidth() - 1 && Shift < 64) {
+        auto Result = decompose(Op0, Preconditions, IsSigned, DL);
+        Result.mul(int64_t(1) << Shift);
+        return Result;
+      }
+    }
+
     return V;
   }
 
diff --git a/llvm/test/Transforms/ConstraintElimination/shl.ll b/llvm/test/Transforms/ConstraintElimination/shl.ll
index 8a00eb9b2830ba..4af54da1f7e817 100644
--- a/llvm/test/Transforms/ConstraintElimination/shl.ll
+++ b/llvm/test/Transforms/ConstraintElimination/shl.ll
@@ -1283,8 +1283,7 @@ define i1 @shl_nsw_x8_slt_x7(i8 %start, i8 %high) {
 ; CHECK-NEXT:    [[C_1:%.*]] = icmp slt i8 [[START_SHL_3]], [[HIGH]]
 ; CHECK-NEXT:    call void @llvm.assume(i1 [[C_1]])
 ; CHECK-NEXT:    [[START_MUL_7:%.*]] = mul nsw i8 [[START]], 7
-; CHECK-NEXT:    [[T_1:%.*]] = icmp slt i8 [[START_MUL_7]], [[HIGH]]
-; CHECK-NEXT:    ret i1 [[T_1]]
+; CHECK-NEXT:    ret i1 true
 ;
   %c.0 = icmp sge i8 %high, 0
   call void @llvm.assume(i1 %c.0)
@@ -1327,11 +1326,9 @@ define i1 @shl_nsw_sign_implication(i8 %x) {
 ; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i8 [[X]], 0
 ; CHECK-NEXT:    br i1 [[CMP1]], label [[IF:%.*]], label [[ELSE:%.*]]
 ; CHECK:       if:
-; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i8 [[SHL]], 0
-; CHECK-NEXT:    ret i1 [[CMP2]]
+; CHECK-NEXT:    ret i1 true
 ; CHECK:       else:
-; CHECK-NEXT:    [[CMP3:%.*]] = icmp sge i8 [[SHL]], 0
-; CHECK-NEXT:    ret i1 [[CMP3]]
+; CHECK-NEXT:    ret i1 true
 ;
   %shl = shl nsw i8 %x, 2
   %cmp1 = icmp slt i8 %x, 0

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jan 4, 2024
dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Jan 4, 2024
Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -517,6 +517,17 @@ static Decomposition decompose(Value *V,
return Result;
}

// shl nsw x, shift is mul nsw x, (1<<shift),
// with the exception of shift == bw-1.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: reflow comment?

@@ -517,6 +517,17 @@ static Decomposition decompose(Value *V,
return Result;
}

// shl nsw x, shift is mul nsw x, (1<<shift),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be good to wrap the expressions in () or something else to separate them from the interleave text

// with the exception of shift == bw-1.
if (match(V, m_NSWShl(m_Value(Op0), m_ConstantInt(CI)))) {
uint64_t Shift = CI->getValue().getLimitedValue();
if (Shift < Ty->getIntegerBitWidth() - 1 && Shift < 64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there's no test case for Shift >= 64 at the moment. IIUC this is to avoid overflows when doing 1 << Shift? We won't reach this code if the types are wider than 64 bit and if the type is <= i64 then shifts >= 64 will produce poison. Might be helpful for future readers to document the condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this check is actually redundant. I'll replace it with an assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

@nikic nikic merged commit 71f56e4 into llvm:main Jan 5, 2024
3 of 4 checks passed
@DaniilSuchkov
Copy link
Contributor

Hi! This change seems to have caused a miscompile: #78621

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.

None yet

4 participants