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

Use range attribute to constant fold comparisons with constant values. #84627

Merged
merged 2 commits into from
Mar 12, 2024

Conversation

andjo403
Copy link
Contributor

@andjo403 andjo403 commented Mar 9, 2024

use the new range attribute from #84617

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 9, 2024

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Andreas Jonson (andjo403)

Changes

use the new range attribute from #84617


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/InstructionSimplify.cpp (+21-17)
  • (modified) llvm/test/Transforms/InstCombine/icmp-range.ll (+29)
diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 201472a3f10c2e..6a709f29db1e33 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -3729,6 +3729,19 @@ static Value *simplifyICmpWithIntrinsicOnLHS(CmpInst::Predicate Pred,
   }
 }
 
+/// Helper method to get range from metadata or attribute.
+static std::optional<ConstantRange> getRange(Value *V,
+                                             const InstrInfoQuery &IIQ) {
+  if (Instruction *I = dyn_cast<Instruction>(V))
+    if (MDNode *MD = IIQ.getMetadata(I, LLVMContext::MD_range))
+      return getConstantRangeFromMetadata(*MD);
+  if (const Argument *A = dyn_cast<Argument>(V))
+    if (A->hasAttribute(llvm::Attribute::Range))
+      return A->getAttribute(llvm::Attribute::Range).getRange();
+
+  return std::nullopt;
+}
+
 /// Given operands for an ICmpInst, see if we can fold the result.
 /// If not, this returns null.
 static Value *simplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
@@ -3776,23 +3789,14 @@ static Value *simplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
 
   // If both operands have range metadata, use the metadata
   // to simplify the comparison.
-  if (isa<Instruction>(RHS) && isa<Instruction>(LHS)) {
-    auto RHS_Instr = cast<Instruction>(RHS);
-    auto LHS_Instr = cast<Instruction>(LHS);
-
-    if (Q.IIQ.getMetadata(RHS_Instr, LLVMContext::MD_range) &&
-        Q.IIQ.getMetadata(LHS_Instr, LLVMContext::MD_range)) {
-      auto RHS_CR = getConstantRangeFromMetadata(
-          *RHS_Instr->getMetadata(LLVMContext::MD_range));
-      auto LHS_CR = getConstantRangeFromMetadata(
-          *LHS_Instr->getMetadata(LLVMContext::MD_range));
-
-      if (LHS_CR.icmp(Pred, RHS_CR))
-        return ConstantInt::getTrue(RHS->getContext());
-
-      if (LHS_CR.icmp(CmpInst::getInversePredicate(Pred), RHS_CR))
-        return ConstantInt::getFalse(RHS->getContext());
-    }
+  std::optional<ConstantRange> RhsCr = getRange(RHS, Q.IIQ);
+  std::optional<ConstantRange> LhsCr = getRange(LHS, Q.IIQ);
+  if (RhsCr && LhsCr) {
+    if (LhsCr->icmp(Pred, *RhsCr))
+      return ConstantInt::getTrue(RHS->getContext());
+
+    if (LhsCr->icmp(CmpInst::getInversePredicate(Pred), *RhsCr))
+      return ConstantInt::getFalse(RHS->getContext());
   }
 
   // Compare of cast, for example (zext X) != 0 -> X != 0
diff --git a/llvm/test/Transforms/InstCombine/icmp-range.ll b/llvm/test/Transforms/InstCombine/icmp-range.ll
index 7af06e03fd4b2a..73da24f69c877a 100644
--- a/llvm/test/Transforms/InstCombine/icmp-range.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-range.ll
@@ -160,6 +160,35 @@ define i1 @test_two_ranges2(ptr nocapture readonly %arg1, ptr nocapture readonly
   ret i1 %rval
 }
 
+; Values' ranges do not overlap each other, so it can simplified to false.
+define i1 @test_two_argument_ranges(i32 range(i32 1, 6) %arg1, i32 range(i32 8, 16) %arg2) {
+; CHECK-LABEL: @test_two_argument_ranges(
+; CHECK-NEXT:    ret i1 false
+;
+  %rval = icmp ult i32 %arg2, %arg1
+  ret i1 %rval
+}
+
+; Values' ranges do not overlap each other, so it can simplified to false.
+define i1 @test_one_range_and_one_argument_range(ptr nocapture readonly %arg1, i32 range(i32 8, 16) %arg2) {
+; CHECK-LABEL: @test_one_range_and_one_argument_range(
+; CHECK-NEXT:    ret i1 false
+;
+  %val1 = load i32, ptr %arg1, !range !0
+  %rval = icmp ult i32 %arg2, %val1
+  ret i1 %rval
+}
+
+; Values' ranges do not overlap each other, so it can simplified to false.
+define i1 @test_one_argument_range_and_one_range(i32 range(i32 1, 6) %arg1, ptr nocapture readonly %arg2) {
+; CHECK-LABEL: @test_one_argument_range_and_one_range(
+; CHECK-NEXT:    ret i1 false
+;
+  %val1 = load i32, ptr %arg2, !range !6
+  %rval = icmp ult i32 %val1, %arg1
+  ret i1 %rval
+}
+
 ; Values' ranges do not overlap each other, so it can simplified to true.
 define i1 @test_two_ranges3(ptr nocapture readonly %arg1, ptr nocapture readonly %arg2) {
 ; CHECK-LABEL: @test_two_ranges3(

@andjo403
Copy link
Contributor Author

andjo403 commented Mar 9, 2024

Will try to check all places that use the range metadata and see if the range attribute also shall be handled.
Is it good to have a PR per use or shall I add more to this?

llvm/lib/Analysis/InstructionSimplify.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/InstructionSimplify.cpp Outdated Show resolved Hide resolved
@nikic
Copy link
Contributor

nikic commented Mar 9, 2024

Will try to check all places that use the range metadata and see if the range attribute also shall be handled. Is it good to have a PR per use or shall I add more to this?

PR per use (or at least per pass) is preferred.

@llvmbot llvmbot added the llvm:ir label Mar 9, 2024
@andjo403
Copy link
Contributor Author

andjo403 commented Mar 9, 2024

fixed the comments

nikic pushed a commit that referenced this pull request Mar 10, 2024
…ange metadata (#84673)

Found that this failed with an assertion when vec was used in this
optimization while working on #84627.
@andjo403 andjo403 force-pushed the ConstantFoldCompareRangeAttribute branch from 38d8170 to dcf1daa Compare March 11, 2024 21:29
@andjo403
Copy link
Contributor Author

did a rebase as I caused a merge conflict with my other commit.
but was able to added test for vec arguments with range attribute now.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

In terms of APIs, It might make sense to add getRange() on Argument/CallBase, which always returns a ConstantRange, using Full range if attribute is not present. Using std::optional with ConstantRange is kind of redundant. But we can see how this goes with further usage.

@nikic nikic merged commit a3b5250 into llvm:main Mar 12, 2024
3 of 4 checks passed
@andjo403 andjo403 deleted the ConstantFoldCompareRangeAttribute branch March 12, 2024 17:05
@andjo403
Copy link
Contributor Author

It might make sense to add getRange() on Argument/CallBase, which always returns a ConstantRange, using Full range if attribute is not present.

it feels a bit strange maybe when the argument/result is e.g. a pointer typ, what shall the bit width be for the Full range in that case?

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

3 participants