[IPSCCP] Add range attribute handling. #86747

andjo403 · 2024-03-26T23:13:39Z

No description provided.

llvmbot · 2024-03-26T23:14:15Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-function-specialization

Author: Andreas Jonson (andjo403)

Changes

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

4 Files Affected:

(modified) llvm/include/llvm/Transforms/Utils/SCCPSolver.h (+4)
(modified) llvm/lib/Transforms/IPO/SCCP.cpp (+1-2)
(modified) llvm/lib/Transforms/Utils/SCCPSolver.cpp (+37-2)
(added) llvm/test/Transforms/SCCP/range-attribute.ll (+82)

diff --git a/llvm/include/llvm/Transforms/Utils/SCCPSolver.h b/llvm/include/llvm/Transforms/Utils/SCCPSolver.h
index 1a95f80812aabd..9f7ccd4a8a32cf 100644
--- a/llvm/include/llvm/Transforms/Utils/SCCPSolver.h
+++ b/llvm/include/llvm/Transforms/Utils/SCCPSolver.h
@@ -151,6 +151,10 @@ class SCCPSolver {
   /// works with both scalars and structs.
   void markOverdefined(Value *V);
 
+  /// trackValueOfArgument - Mark the specified argument overdefined unless it
+  /// have range attribute.  This works with both scalars and structs.
+  void trackValueOfArgument(Argument *V);
+
   // isStructLatticeConstant - Return true if all the lattice values
   // corresponding to elements of the structure are constants,
   // false otherwise.
diff --git a/llvm/lib/Transforms/IPO/SCCP.cpp b/llvm/lib/Transforms/IPO/SCCP.cpp
index b1f9b827dcbaf3..f8920541e6fd64 100644
--- a/llvm/lib/Transforms/IPO/SCCP.cpp
+++ b/llvm/lib/Transforms/IPO/SCCP.cpp
@@ -144,9 +144,8 @@ static bool runIPSCCP(
     // Assume the function is called.
     Solver.markBlockExecutable(&F.front());
 
-    // Assume nothing about the incoming arguments.
     for (Argument &AI : F.args())
-      Solver.markOverdefined(&AI);
+      Solver.trackValueOfArgument(&AI);
   }
 
   // Determine if we can track any of the module's global variables. If so, add
diff --git a/llvm/lib/Transforms/Utils/SCCPSolver.cpp b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
index 38fc7763c5b204..4867c7b204c841 100644
--- a/llvm/lib/Transforms/Utils/SCCPSolver.cpp
+++ b/llvm/lib/Transforms/Utils/SCCPSolver.cpp
@@ -428,6 +428,12 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
     return markConstant(ValueState[V], V, C);
   }
 
+  /// markConstantRange - Mark the object as constant range with \p CR. If the
+  /// object is not a constant range with the range \p CR, add it to the
+  /// instruction work list so that the users of the instruction are updated
+  /// later.
+  bool markConstantRange(ValueLatticeElement &IV, Value *V, ConstantRange &CR);
+
   // markOverdefined - Make a value be marked as "overdefined". If the
   // value is not already overdefined, add it to the overdefined instruction
   // work list so that the users of the instruction are updated later.
@@ -788,6 +794,17 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
       markOverdefined(ValueState[V], V);
   }
 
+  void trackValueOfArgument(Argument *A) {
+    if (A->getType()->isIntegerTy()) {
+      if (std::optional<ConstantRange> Range = A->getRange()) {
+        markConstantRange(ValueState[A], A, *Range);
+        return;
+      }
+    }
+    // Assume nothing about the incoming arguments without range.
+    markOverdefined(A);
+  }
+
   bool isStructLatticeConstant(Function *F, StructType *STy);
 
   Constant *getConstant(const ValueLatticeElement &LV, Type *Ty) const;
@@ -873,6 +890,15 @@ bool SCCPInstVisitor::markConstant(ValueLatticeElement &IV, Value *V,
   return true;
 }
 
+bool SCCPInstVisitor::markConstantRange(ValueLatticeElement &IV, Value *V,
+                                        ConstantRange &CR) {
+  if (!IV.markConstantRange(CR))
+    return false;
+  LLVM_DEBUG(dbgs() << "markConstantRange: " << CR << ": " << *V << '\n');
+  pushToWorkList(IV, V);
+  return true;
+}
+
 bool SCCPInstVisitor::markOverdefined(ValueLatticeElement &IV, Value *V) {
   if (!IV.markOverdefined())
     return false;
@@ -1581,10 +1607,15 @@ void SCCPInstVisitor::visitStoreInst(StoreInst &SI) {
 }
 
 static ValueLatticeElement getValueFromMetadata(const Instruction *I) {
-  if (MDNode *Ranges = I->getMetadata(LLVMContext::MD_range))
-    if (I->getType()->isIntegerTy())
+  if (I->getType()->isIntegerTy()) {
+    if (MDNode *Ranges = I->getMetadata(LLVMContext::MD_range))
       return ValueLatticeElement::getRange(
           getConstantRangeFromMetadata(*Ranges));
+
+    if (const auto *CB = dyn_cast<CallBase>(I))
+      if (std::optional<ConstantRange> Range = CB->getRange())
+        return ValueLatticeElement::getRange(*Range);
+  }
   if (I->hasMetadata(LLVMContext::MD_nonnull))
     return ValueLatticeElement::getNot(
         ConstantPointerNull::get(cast<PointerType>(I->getType())));
@@ -2090,6 +2121,10 @@ const SmallPtrSet<Function *, 16> SCCPSolver::getMRVFunctionsTracked() {
 
 void SCCPSolver::markOverdefined(Value *V) { Visitor->markOverdefined(V); }
 
+void SCCPSolver::trackValueOfArgument(Argument *V) {
+  Visitor->trackValueOfArgument(V);
+}
+
 bool SCCPSolver::isStructLatticeConstant(Function *F, StructType *STy) {
   return Visitor->isStructLatticeConstant(F, STy);
 }
diff --git a/llvm/test/Transforms/SCCP/range-attribute.ll b/llvm/test/Transforms/SCCP/range-attribute.ll
new file mode 100644
index 00000000000000..597f9246a577d7
--- /dev/null
+++ b/llvm/test/Transforms/SCCP/range-attribute.ll
@@ -0,0 +1,82 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=ipsccp -S | FileCheck %s
+
+declare void @use(i1)
+declare i32 @get_i32()
+
+define void @range_attribute(i32 range(i32 0, 10) %v) {
+; CHECK-LABEL: @range_attribute(
+; CHECK-NEXT:    call void @use(i1 true)
+; CHECK-NEXT:    [[C2:%.*]] = icmp ult i32 [[V:%.*]], 9
+; CHECK-NEXT:    call void @use(i1 [[C2]])
+; CHECK-NEXT:    call void @use(i1 false)
+; CHECK-NEXT:    [[C4:%.*]] = icmp ugt i32 [[V]], 8
+; CHECK-NEXT:    call void @use(i1 [[C4]])
+; CHECK-NEXT:    ret void
+;
+  %c1 = icmp ult i32 %v, 10
+  call void @use(i1 %c1)
+  %c2 = icmp ult i32 %v, 9
+  call void @use(i1 %c2)
+  %c3 = icmp ugt i32 %v, 9
+  call void @use(i1 %c3)
+  %c4 = icmp ugt i32 %v, 8
+  call void @use(i1 %c4)
+  ret void
+}
+
+define i32 @range_attribute_single(i32 range(i32 0, 1) %v) {
+; CHECK-LABEL: @range_attribute_single(
+; CHECK-NEXT:    ret i32 0
+;
+  ret i32 %v
+}
+
+define void @call_range_attribute(ptr %p) {
+; CHECK-LABEL: @call_range_attribute(
+; CHECK-NEXT:    [[V:%.*]] = call range(i32 0, 10) i32 @get_i32()
+; CHECK-NEXT:    call void @use(i1 true)
+; CHECK-NEXT:    [[C2:%.*]] = icmp ult i32 [[V]], 9
+; CHECK-NEXT:    call void @use(i1 [[C2]])
+; CHECK-NEXT:    call void @use(i1 false)
+; CHECK-NEXT:    [[C4:%.*]] = icmp ugt i32 [[V]], 8
+; CHECK-NEXT:    call void @use(i1 [[C4]])
+; CHECK-NEXT:    ret void
+;
+  %v = call range(i32 0, 10) i32 @get_i32()
+  %c1 = icmp ult i32 %v, 10
+  call void @use(i1 %c1)
+  %c2 = icmp ult i32 %v, 9
+  call void @use(i1 %c2)
+  %c3 = icmp ugt i32 %v, 9
+  call void @use(i1 %c3)
+  %c4 = icmp ugt i32 %v, 8
+  call void @use(i1 %c4)
+  ret void
+}
+
+
+declare range(i32 0, 10) i32 @get_i32_in_range()
+
+define void @call_range_result(ptr %p) {
+; CHECK-LABEL: @call_range_result(
+; CHECK-NEXT:    [[V:%.*]] = call i32 @get_i32_in_range()
+; CHECK-NEXT:    call void @use(i1 true)
+; CHECK-NEXT:    [[C2:%.*]] = icmp ult i32 [[V]], 9
+; CHECK-NEXT:    call void @use(i1 [[C2]])
+; CHECK-NEXT:    call void @use(i1 false)
+; CHECK-NEXT:    [[C4:%.*]] = icmp ugt i32 [[V]], 8
+; CHECK-NEXT:    call void @use(i1 [[C4]])
+; CHECK-NEXT:    ret void
+;
+  %v = call i32 @get_i32_in_range()
+  %c1 = icmp ult i32 %v, 10
+  call void @use(i1 %c1)
+  %c2 = icmp ult i32 %v, 9
+  call void @use(i1 %c2)
+  %c3 = icmp ugt i32 %v, 9
+  call void @use(i1 %c3)
+  %c4 = icmp ugt i32 %v, 8
+  call void @use(i1 %c4)
+  ret void
+}

andjo403 · 2024-03-27T07:45:48Z

CC @nikic as you seems to have done changes here also for the range metadata.

antoniofrighetto

LGTM, markConstantRange implementation looks good. Let's wait for @nikic as well when he's back though.

dtcxzyw · 2024-03-27T09:30:41Z

llvm/include/llvm/Transforms/Utils/SCCPSolver.h

@@ -151,6 +151,10 @@ class SCCPSolver {
  /// works with both scalars and structs.
  void markOverdefined(Value *V);

+  /// trackValueOfArgument - Mark the specified argument overdefined unless it
+  /// have range attribute.  This works with both scalars and structs.


This works with both scalars and structs.

I don't understand why it works with structs. Doesn't it only work with integer scalars?

as it calls the markOverdefined if it is not a integer with a range attribute and that function have that text I selected to have it for this function also.

nikic · 2024-03-27T12:15:53Z

llvm/lib/Transforms/Utils/SCCPSolver.cpp

@@ -788,6 +794,17 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
      markOverdefined(ValueState[V], V);
  }

+  void trackValueOfArgument(Argument *A) {
+    if (A->getType()->isIntegerTy()) {


Do we need the restriction to non-vectors here? IIRC ipsccp can handle vectors though it mostly doesn't.

I only assumed that vectors was not handled as there was a restriction to only non-vectors for the range metadata.

Ok, we can relax this separately from this PR.

Tried to add a test for vector like below but was not optimized when I remove the check. But maybe I did something wrong.

define void @range_attribute_vec(<2 x i32> range(i32 0, 10) %v) { %c1 = icmp ult <2 x i32> %v, <i32 10, i32 10> call void @use(<2 x i1> %c1) %c2 = icmp ult <2 x i32> %v, <i32 9, i32 9> call void @use(<2 x i1> %c2) %c3 = icmp ugt <2 x i32> %v, <i32 9, i32 9> call void @use(<2 x i1> %c3) %c4 = icmp ugt <2 x i32> %v, <i32 8, i32 8> call void @use(<2 x i1> %c4) ret void }

think that my test fails as ConstantVector is not handled in https://github.com/andjo403/llvm-project/blob/fa71860e1937b916758693e268d5a16021c8b5c3/llvm/include/llvm/Analysis/ValueLattice.h#L301-L339

nikic · 2024-03-27T12:17:10Z

llvm/test/Transforms/SCCP/range-attribute.ll

+  %c4 = icmp ugt i32 %v, 8
+  call void @use(i1 %c4)
+  ret void
+}


Can you please also check that range() does not pessimize inter-procedural inference? I.e. if we get better info from call-sites, we should use it.

added test for this.

This isn't quite what I was looking for, or maybe I'm misunderstanding the test. What I had in mind if a function with range(0, 10) that has icmp 5 and is only called with value 5, or something like that.

ok I thought that you was thinking of the test ip_load_range that exist for range metadata
https://github.com/andjo403/llvm-project/blob/fff4a199f2b1286618ee129726a71e753c0b7ea1/llvm/test/Transforms/SCCP/metadata.ll#L102-L119

So I think I did kind of the opposite of the test you was thinking of, that the range is propagated in to a function without a range and you want a constant to be propagated in to a function with a argument that have a range if I understand correctly.

nikic · 2024-04-10T06:37:47Z

llvm/lib/Transforms/Utils/SCCPSolver.cpp

@@ -788,6 +794,17 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
      markOverdefined(ValueState[V], V);
  }

+  void trackValueOfArgument(Argument *A) {
+    if (A->getType()->isIntegerTy()) {


Ok, we can relax this separately from this PR.

nikic · 2024-04-10T06:38:47Z

llvm/lib/Transforms/Utils/SCCPSolver.cpp

@@ -873,6 +890,15 @@ bool SCCPInstVisitor::markConstant(ValueLatticeElement &IV, Value *V,
  return true;
 }

+bool SCCPInstVisitor::markConstantRange(ValueLatticeElement &IV, Value *V,
+                                        ConstantRange &CR) {


Should pass by const reference here (or by value and use std::move, but that's not compatible with the dbg printing).

added const

nikic · 2024-04-10T06:42:10Z

llvm/test/Transforms/SCCP/range-attribute.ll

+  %c4 = icmp ugt i32 %v, 8
+  call void @use(i1 %c4)
+  ret void
+}


This isn't quite what I was looking for, or maybe I'm misunderstanding the test. What I had in mind if a function with range(0, 10) that has icmp 5 and is only called with value 5, or something like that.

nikic

LGTM

llvmbot added function-specialization llvm:transforms labels Mar 26, 2024

XChy requested review from nikic, dtcxzyw and antoniofrighetto March 27, 2024 07:47

antoniofrighetto approved these changes Mar 27, 2024

View reviewed changes

dtcxzyw reviewed Mar 27, 2024

View reviewed changes

nikic reviewed Mar 27, 2024

View reviewed changes

andjo403 force-pushed the sccpSolverRangeAttribute branch from f7eef0e to fff4a19 Compare April 4, 2024 19:33

nikic reviewed Apr 10, 2024

View reviewed changes

andjo403 added 2 commits April 10, 2024 20:56

[IPSCCP] add test for range attribute handling. NFC.

9f2fb2a

[IPSCCP] Add range attribute handling.

fa71860

andjo403 force-pushed the sccpSolverRangeAttribute branch from fff4a19 to fa71860 Compare April 10, 2024 19:08

nikic approved these changes Apr 11, 2024

View reviewed changes

nikic merged commit e7bc537 into llvm:main Apr 11, 2024
4 checks passed

andjo403 deleted the sccpSolverRangeAttribute branch April 11, 2024 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IPSCCP] Add range attribute handling. #86747

[IPSCCP] Add range attribute handling. #86747

andjo403 commented Mar 26, 2024

llvmbot commented Mar 26, 2024 •

edited

andjo403 commented Mar 27, 2024

antoniofrighetto left a comment

dtcxzyw Mar 27, 2024

andjo403 Mar 27, 2024

nikic Mar 27, 2024

andjo403 Mar 27, 2024

nikic Apr 10, 2024

andjo403 Apr 10, 2024

andjo403 Apr 10, 2024

nikic Mar 27, 2024

andjo403 Apr 4, 2024

nikic Apr 10, 2024

andjo403 Apr 10, 2024

nikic Apr 10, 2024

nikic Apr 10, 2024

andjo403 Apr 10, 2024

nikic Apr 10, 2024

nikic left a comment

[IPSCCP] Add range attribute handling. #86747

[IPSCCP] Add range attribute handling. #86747

Conversation

andjo403 commented Mar 26, 2024

llvmbot commented Mar 26, 2024 • edited

andjo403 commented Mar 27, 2024

antoniofrighetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

llvmbot commented Mar 26, 2024 •

edited