Skip to content

[HLSL] Add support to lookup a ResourceBindingInfo from its use #126556

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 12 commits into from
Feb 19, 2025
Merged

[HLSL] Add support to lookup a ResourceBindingInfo from its use #126556

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ab1e78d
Implement trivial resource lookup with test
V-FEXrt Feb 7, 2025
e7d1042
format
V-FEXrt Feb 10, 2025
09b6373
Add TODOs
V-FEXrt Feb 10, 2025
2039b3b
Fully implement and add tests
V-FEXrt Feb 14, 2025
f3e3b3f
format
V-FEXrt Feb 14, 2025
cd70254
Fix warnings
V-FEXrt Feb 14, 2025
d2f17eb
address comments
V-FEXrt Feb 18, 2025
2ac7284
Address comments
V-FEXrt Feb 18, 2025
c17776b
format
V-FEXrt Feb 18, 2025
48a0201
Merge branch 'main' into hlsl-125746-find-resource-by-use
V-FEXrt Feb 18, 2025
010c535
address comments
V-FEXrt Feb 18, 2025
2385cef
format
V-FEXrt Feb 18, 2025
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
7 changes: 5 additions & 2 deletions llvm/include/llvm/Analysis/DXILResource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,8 +446,11 @@ class DXILBindingMap {
return Pos == CallMap.end() ? Infos.end() : (Infos.begin() + Pos->second);
}

// Resoloves the use of a resource handle into the unique description of that
// resource by deduping calls to create.
/// Resolves a resource handle into a vector of ResourceBindingInfos that
/// represent the possible unique creations of the handle. Certain cases are
/// ambiguous so mulitple creation points may be returned. The resulting
/// ResourceBindingInfo can be used to depuplicate unique handles that
/// reference the same resource
SmallVector<dxil::ResourceBindingInfo> findByUse(const Value *Key) const;

const_iterator find(const CallInst *Key) const {
Expand Down
12 changes: 4 additions & 8 deletions llvm/lib/Analysis/DXILResource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,7 @@ void DXILBindingMap::print(raw_ostream &OS, DXILResourceTypeMap &DRTM,

SmallVector<dxil::ResourceBindingInfo>
DXILBindingMap::findByUse(const Value *Key) const {
const PHINode *Phi = dyn_cast<PHINode>(Key);
if (Phi) {
if (const PHINode *Phi = dyn_cast<PHINode>(Key)) {
SmallVector<dxil::ResourceBindingInfo> Children;
for (const Value *V : Phi->operands()) {
Children.append(findByUse(V));
Copy link
Contributor

Choose a reason for hiding this comment

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

Without a set that tracks and skips already visited Value* pointers, this could result in infinite recursion, right?

Copy link
Contributor Author

@V-FEXrt V-FEXrt Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Uhhh maybe, but I dont think so? Can you formulate an example? I've been trying myself for a bit and everything I come up with violates SSA.

Visited lists are only relevant when cycles are possible and in order to introduce a cycle we need a CallInstr to reference something not yet defined and that's not allowed right?

%bat = call @foo %bar
%bar = call @foo2 %bat

Expand All @@ -782,9 +781,8 @@ DXILBindingMap::findByUse(const Value *Key) const {
}

const CallInst *CI = dyn_cast<CallInst>(Key);
Copy link
Member

Choose a reason for hiding this comment

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

How does it impact the recursion if we check for CallInst before PHINode? I kind of like my base cases to be first, but I know thats not always possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to test it but it looks like PHINode isn't a subclass of CallInst right? https://llvm.org/doxygen/classllvm_1_1PHINode.html

Which means the base case would earily exit before the PHINode code had a chance to run. Could maybe tuck it inside the if (!CI) but that feels a bit messsy to me

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 the trivial reorder causes the PHINodes case to fail

if (!CI) {
if (!CI)
return {};
}

const Type *UseType = CI->getType();

Expand All @@ -794,9 +792,8 @@ DXILBindingMap::findByUse(const Value *Key) const {
case Intrinsic::not_intrinsic: {
SmallVector<dxil::ResourceBindingInfo> Children;
for (const Value *V : CI->args()) {
if (V->getType() != UseType) {
if (V->getType() != UseType)
continue;
}

Children.append(findByUse(V));
}
Expand All @@ -806,8 +803,7 @@ DXILBindingMap::findByUse(const Value *Key) const {
// Found the create, return the binding
case Intrinsic::dx_resource_handlefrombinding:
const auto *It = find(CI);
if (It == Infos.end())
return {};
assert(It != Infos.end() && "HandleFromBinding must be in resource map");
return {*It};
}

Expand Down
20 changes: 4 additions & 16 deletions llvm/unittests/Target/DirectX/UniqueResourceFromUseTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,8 @@ declare void @a.func(target("dx.RawBuffer", float, 1, 0) %handle)
unsigned CalledResources = 0;

for (const User *U : F.users()) {
const CallInst *CI = dyn_cast<CallInst>(U);
ASSERT_TRUE(CI) << "All users of @a.func must be CallInst";

const CallInst *CI = cast<CallInst>(U);
const Value *Handle = CI->getArgOperand(0);

const auto Bindings = DBM.findByUse(Handle);
ASSERT_EQ(Bindings.size(), 1u)
<< "Handle should resolve into one resource";
Expand Down Expand Up @@ -124,11 +121,8 @@ declare target("dx.RawBuffer", float, 1, 0) @ind.func(target("dx.RawBuffer", flo
unsigned CalledResources = 0;

for (const User *U : F.users()) {
const CallInst *CI = dyn_cast<CallInst>(U);
ASSERT_TRUE(CI) << "All users of @a.func must be CallInst";

const CallInst *CI = cast<CallInst>(U);
const Value *Handle = CI->getArgOperand(0);

const auto Bindings = DBM.findByUse(Handle);
ASSERT_EQ(Bindings.size(), 1u)
<< "Handle should resolve into one resource";
Expand Down Expand Up @@ -180,11 +174,8 @@ declare target("dx.RawBuffer", float, 1, 0) @ind.func(target("dx.RawBuffer", flo
unsigned CalledResources = 0;

for (const User *U : F.users()) {
const CallInst *CI = dyn_cast<CallInst>(U);
ASSERT_TRUE(CI) << "All users of @a.func must be CallInst";

const CallInst *CI = cast<CallInst>(U);
const Value *Handle = CI->getArgOperand(0);

const auto Bindings = DBM.findByUse(Handle);
ASSERT_EQ(Bindings.size(), 4u)
<< "Handle should resolve into four resources";
Expand Down Expand Up @@ -263,11 +254,8 @@ declare target("dx.RawBuffer", float, 1, 0) @ind.func(target("dx.RawBuffer", flo
unsigned CalledResources = 0;

for (const User *U : F.users()) {
const CallInst *CI = dyn_cast<CallInst>(U);
ASSERT_TRUE(CI) << "All users of @a.func must be CallInst";

const CallInst *CI = cast<CallInst>(U);
const Value *Handle = CI->getArgOperand(0);

const auto Bindings = DBM.findByUse(Handle);
ASSERT_EQ(Bindings.size(), 2u)
<< "Handle should resolve into four resources";
Expand Down
Loading