Skip to content

[IPSCCP] Add range attribute handling. #86747

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

Merged
merged 2 commits into from
Apr 11, 2024
Merged

[IPSCCP] Add range attribute handling. #86747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f2fb2a
[IPSCCP] add test for range attribute handling. NFC.
andjo403 Apr 4, 2024
fa71860
[IPSCCP] Add range attribute handling.
andjo403 Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/include/llvm/Transforms/Utils/SCCPSolver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

This works with both scalars and structs.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

void trackValueOfArgument(Argument *V);

// isStructLatticeConstant - Return true if all the lattice values
// corresponding to elements of the structure are constants,
// false otherwise.
Expand Down
3 changes: 1 addition & 2 deletions llvm/lib/Transforms/IPO/SCCP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 38 additions & 2 deletions llvm/lib/Transforms/Utils/SCCPSolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,13 @@ 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,
const 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.
Expand Down Expand Up @@ -788,6 +795,17 @@ class SCCPInstVisitor : public InstVisitor<SCCPInstVisitor> {
markOverdefined(ValueState[V], V);
}

void trackValueOfArgument(Argument *A) {
if (A->getType()->isIntegerTy()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can relax this separately from this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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;
Expand Down Expand Up @@ -873,6 +891,15 @@ bool SCCPInstVisitor::markConstant(ValueLatticeElement &IV, Value *V,
return true;
}

bool SCCPInstVisitor::markConstantRange(ValueLatticeElement &IV, Value *V,
const 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;
Expand Down Expand Up @@ -1581,10 +1608,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())));
Expand Down Expand Up @@ -2090,6 +2122,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);
}
Expand Down
154 changes: 154 additions & 0 deletions llvm/test/Transforms/SCCP/range-attribute.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
; 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() {
; 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() {
; 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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added test for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


define internal i1 @ip_cmp_range_attribute(i32 %v) {
; CHECK-LABEL: @ip_cmp_range_attribute(
; CHECK-NEXT: ret i1 undef
;
%c = icmp ult i32 %v, 10
ret i1 %c
}

define i1 @ip_range_attribute(i32 range(i32 0, 10) %v) {
; CHECK-LABEL: @ip_range_attribute(
; CHECK-NEXT: [[C:%.*]] = call i1 @ip_cmp_range_attribute(i32 [[V:%.*]])
; CHECK-NEXT: ret i1 true
;
%c = call i1 @ip_cmp_range_attribute(i32 %v)
ret i1 %c
}

define internal i1 @ip_cmp_range_call(i32 %v) {
; CHECK-LABEL: @ip_cmp_range_call(
; CHECK-NEXT: ret i1 undef
;
%c = icmp ult i32 %v, 10
ret i1 %c
}

define i1 @ip_range_call() {
; CHECK-LABEL: @ip_range_call(
; CHECK-NEXT: [[V:%.*]] = call range(i32 0, 10) i32 @get_i32()
; CHECK-NEXT: [[C:%.*]] = call i1 @ip_cmp_range_call(i32 [[V]])
; CHECK-NEXT: ret i1 true
;
%v = call range(i32 0, 10) i32 @get_i32()
%c = call i1 @ip_cmp_range_call(i32 %v)
ret i1 %c
}

define internal i1 @ip_cmp_range_result(i32 %v) {
; CHECK-LABEL: @ip_cmp_range_result(
; CHECK-NEXT: ret i1 undef
;
%c = icmp ult i32 %v, 10
ret i1 %c
}

define i1 @ip_range_result() {
; CHECK-LABEL: @ip_range_result(
; CHECK-NEXT: [[V:%.*]] = call range(i32 0, 10) i32 @get_i32()
; CHECK-NEXT: [[C:%.*]] = call i1 @ip_cmp_range_result(i32 [[V]])
; CHECK-NEXT: ret i1 true
;
%v = call range(i32 0, 10) i32 @get_i32()
%c = call i1 @ip_cmp_range_result(i32 %v)
ret i1 %c
}

define internal i1 @ip_cmp_with_range_attribute(i32 range(i32 0, 10) %v) {
; CHECK-LABEL: @ip_cmp_with_range_attribute(
; CHECK-NEXT: ret i1 undef
;
%c = icmp eq i32 %v, 5
ret i1 %c
}

define i1 @ip_range_attribute_constant() {
; CHECK-LABEL: @ip_range_attribute_constant(
; CHECK-NEXT: [[C:%.*]] = call i1 @ip_cmp_with_range_attribute(i32 5)
; CHECK-NEXT: ret i1 true
;
%c = call i1 @ip_cmp_with_range_attribute(i32 5)
ret i1 %c
}