Skip to content

[GVN] Look through select/phi when determining underlying object #99509

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 7 commits into from
Jul 22, 2024
Merged

[GVN] Look through select/phi when determining underlying object #99509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f98349d
[GVN] Look through select/phi when determining underlying object
nikic Jul 15, 2024
291ffd2
Rename to getUnderlyingObjectAggressive
nikic Jul 18, 2024
25857e8
Remove unused MaxLookup parameter
nikic Jul 18, 2024
f160e9c
Refactor FirstObject logic
nikic Jul 22, 2024
8e5f7b8
Revert "Refactor FirstObject logic"
nikic Jul 22, 2024
cac98c1
Use bool First instead
nikic Jul 22, 2024
44714f0
Remove leftover code
nikic Jul 22, 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/Analysis/ValueTracking.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,10 @@ inline Value *getUnderlyingObject(Value *V, unsigned MaxLookup = 6) {
return const_cast<Value *>(getUnderlyingObject(VConst, MaxLookup));
}

/// Like getUnderlyingObject(), but will try harder to find a single underlying
/// object. In particular, this function also looks through selects and phis.
const Value *getUnderlyingObjectAggressive(const Value *V);
Copy link

Choose a reason for hiding this comment

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

Could you still expose a parameter to control the aggressiveness? Aggressive without a parameter is kind of scary. E.g. the size of the visited set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we should not expose unnecessary options. If we have reason to have different levels of aggressiveness for different callers in the future, we will add the option at the time. Until then, cutoffs should be centrally controlled.


/// This method is similar to getUnderlyingObject except that it can
/// look through phi and select instructions and return multiple objects.
///
Expand Down
5 changes: 2 additions & 3 deletions llvm/lib/Analysis/Loads.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,9 +743,8 @@ static bool isPointerAlwaysReplaceable(const Value *From, const Value *To,
if (isa<Constant>(To) &&
isDereferenceablePointer(To, Type::getInt8Ty(To->getContext()), DL))
return true;
if (getUnderlyingObject(From) == getUnderlyingObject(To))
return true;
return false;
return getUnderlyingObjectAggressive(From) ==
getUnderlyingObjectAggressive(To);
}

bool llvm::canReplacePointersInUseIfEqual(const Use &U, const Value *To,
Expand Down
42 changes: 42 additions & 0 deletions llvm/lib/Analysis/ValueTracking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6609,6 +6609,48 @@ void llvm::getUnderlyingObjects(const Value *V,
} while (!Worklist.empty());
}

const Value *llvm::getUnderlyingObjectAggressive(const Value *V) {
const unsigned MaxVisited = 8;

SmallPtrSet<const Value *, 8> Visited;
SmallVector<const Value *, 8> Worklist;
Worklist.push_back(V);
const Value *Object = nullptr;
// Used as fallback if we can't find a common underlying object through
// recursion.
bool First = true;
const Value *FirstObject = getUnderlyingObject(V);
do {
const Value *P = Worklist.pop_back_val();
P = First ? FirstObject : getUnderlyingObject(P);
First = false;

if (!Visited.insert(P).second)
continue;

if (Visited.size() == MaxVisited)
return FirstObject;

if (auto *SI = dyn_cast<SelectInst>(P)) {
Worklist.push_back(SI->getTrueValue());
Worklist.push_back(SI->getFalseValue());
continue;
}

if (auto *PN = dyn_cast<PHINode>(P)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the loop case, there's also a variant of llvm::getUnderlyingObjects( that tries to identify PHIs that track the same underlying object I think (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ValueTracking.cpp#L6589)

Would that help/Should this be unified?

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 believe that we don't need that isSameUnderlyingObjectInLoop() logic as long as we're reducing down to a single underlying object. What this handles is the case where you have underlying objects from different loop iterations that have the same SSA value -- this is the same basic issue as "value equal in potential cycles" in BasicAA.

However, if we're reducing to a single object, then this can't happen, because we have both a contribution from the pre-header and the loop. The loop contribution has to reduce back to the loop phi to end up with a single object. If it reduces to a loop variant value, then we'd have at least two underlying objects.

append_range(Worklist, PN->incoming_values());
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe there should be a check that PN->getNumIncominig() < MaxVisisted. Seems a bit inefficient to essentially wait until you have analyzed MaxVisited edges to give up.

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 implemented, a phi with more than MaxVisited could be accepted as long as some of the arguments reduce to the same object (as returned by getUnderlyingObject).

Though I'd be open to make this a more strict limit on the number of getUnderlyingObject calls, regardless of what they return.

I mainly want to make sure we don't run into pathological cases. The cost of this analysis seems to be negligible for the average case.

continue;
}

if (!Object)
Object = P;
else if (Object != P)
return FirstObject;
} while (!Worklist.empty());

return Object;
}

/// This is the function that does the work of looking through basic
/// ptrtoint+arithmetic+inttoptr sequences.
static const Value *getUnderlyingObjectFromInt(const Value *V) {
Expand Down
6 changes: 3 additions & 3 deletions llvm/test/Transforms/GVN/condprop.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,7 @@ define void @select_same_obj(i1 %c, ptr %p, i64 %x) {
; CHECK-NEXT: br i1 [[CMP]], label [[IF:%.*]], label [[EXIT:%.*]]
; CHECK: if:
; CHECK-NEXT: call void @use_ptr(ptr [[P]])
; CHECK-NEXT: call void @use_ptr(ptr [[P3]])
; CHECK-NEXT: call void @use_ptr(ptr [[P]])
; CHECK-NEXT: ret void
; CHECK: exit:
; CHECK-NEXT: ret void
Expand Down Expand Up @@ -902,7 +902,7 @@ define void @phi_same_obj(i1 %c, ptr %p, i64 %x) {
; CHECK-NEXT: br i1 [[CMP]], label [[IF2:%.*]], label [[EXIT:%.*]]
; CHECK: if2:
; CHECK-NEXT: call void @use_ptr(ptr [[P]])
; CHECK-NEXT: call void @use_ptr(ptr [[P3]])
; CHECK-NEXT: call void @use_ptr(ptr [[P]])
; CHECK-NEXT: ret void
; CHECK: exit:
; CHECK-NEXT: ret void
Expand Down Expand Up @@ -975,7 +975,7 @@ define void @phi_same_obj_cycle(i1 %c, ptr %p, i64 %x) {
; CHECK-NEXT: [[CMP:%.*]] = icmp eq ptr [[P_IV]], [[P]]
; CHECK-NEXT: br i1 [[CMP]], label [[IF:%.*]], label [[LOOP_LATCH]]
; CHECK: if:
; CHECK-NEXT: call void @use_ptr(ptr [[P_IV]])
; CHECK-NEXT: call void @use_ptr(ptr [[P]])
; CHECK-NEXT: call void @use_ptr(ptr [[P]])
; CHECK-NEXT: br label [[LOOP_LATCH]]
; CHECK: loop.latch:
Expand Down
Loading