Skip to content

Commit 9536d2f

Browse files
skiminki-nvslangbot
skiminki-nv
and
slangbot
authored
Fix rewriteValueUsesToAddrUses and add regression test (#9113)
Function rewriteValueUsesToAddrUses() translates IRFieldExtract/IRGetElement/IRStore instructions that are affected by the change of the function parameter type from value to pointer. However, the implementation failed to consider the case where: - The base of translated IRGetElement() is IRFieldExtract(); and - The index of translated IRGetElement() is IRFieldExtract() or another IRGetElement() In the problem case, the index would be passed a pointer instead of a dereferenced pointer. To fix this, rewrite the function with a slightly different strategy: - Start by injecting IRLoad() to all uses of the transformed function parameter. - Then, follow up with the injected IRLoad() ops: - Transform IRFieldExtract(IRLoad(ptr), ...) and IRGetElement(IRLoad(ptr), ...) patterns to IRLoad(IRFieldAddress(ptr, ...)) and IRLoad(IRGetElementPtr(ptr, ...)), and keep following up with the newly injected IRLoad() ops. - Transform also IRStore(ptr, load(x))+IRCall(func, ptr, ...) to IRCall(func, x, ...) where ptr is a single use temporary previously added by updateCallSites() Add also a minimal repro for the aforementioned case as a regression test. Issue #9073 --------- Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>
1 parent 4280f24 commit 9536d2f

File tree

2 files changed

+166
-48
lines changed

2 files changed

+166
-48
lines changed

‎source/slang/slang-ir-transform-params-to-constref.cpp‎

Lines changed: 99 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -43,79 +43,130 @@ struct TransformParamsToConstRefContext
4343

4444
void rewriteValueUsesToAddrUses(IRInst* newAddrInst)
4545
{
46-
HashSet<IRInst*> workListSet;
47-
workListSet.add(newAddrInst);
48-
List<IRInst*> workList;
49-
workList.add(newAddrInst);
50-
auto _addToWorkList = [&](IRInst* inst)
51-
{
52-
if (workListSet.add(inst))
53-
workList.add(inst);
54-
};
46+
// The overall strategy here is as follows:
47+
//
48+
// - First, insert IRLoad() in front of all uses of newAddrInst. This
49+
// is a value parameter turned into pointer to value. Add all IRLoad()
50+
// instructions in the working set
51+
// - Then, for every inserted IRLoad() instruction, search for
52+
// IRFieldExtract(IRLoad(ptr), ...) and IRGetElement(IRLoad(ptr), ...) patterns,
53+
// and transform these to IRLoad(IRFieldAddress(ptr, ...)) and
54+
// IRLoad(IRGetElementPtr(ptr, ...)), and insert the new IRLoad()
55+
// instructions in the working set
56+
// - Remove also stores to write-once temporary variables that are
57+
// immediately passed into a constref location in a call (see below)
58+
// - If all uses of the inserted IRLoad() were translated, remove the
59+
// IRLoad() to keep this pass clean
60+
61+
List<IRLoad*> workList;
62+
63+
traverseUses(
64+
newAddrInst,
65+
[&](IRUse* use)
66+
{
67+
auto user = use->getUser();
68+
builder.setInsertBefore(user);
69+
IRLoad* loadInst = as<IRLoad>(builder.emitLoad(newAddrInst));
70+
use->set(loadInst);
71+
72+
workList.add(loadInst);
73+
});
74+
5575
for (Index i = 0; i < workList.getCount(); i++)
5676
{
57-
auto inst = workList[i];
77+
IRLoad* loadInst = workList[i];
78+
bool allUsesTranslated = true;
79+
5880
traverseUses(
59-
inst,
81+
loadInst,
6082
[&](IRUse* use)
6183
{
62-
auto user = use->getUser();
63-
if (workListSet.contains(user))
64-
return;
65-
switch (user->getOp())
84+
IRInst* userInst = use->getUser();
85+
bool useTranslated = false;
86+
87+
switch (userInst->getOp())
6688
{
6789
case kIROp_FieldExtract:
6890
{
69-
// Transform the IRFieldExtract into a IRFieldAddress
70-
if (!isUseBaseAddrOperand(use, user))
71-
break;
72-
auto fieldExtract = as<IRFieldExtract>(user);
73-
builder.setInsertBefore(fieldExtract);
74-
auto fieldAddr = builder.emitFieldAddress(
75-
fieldExtract->getBase(),
76-
fieldExtract->getField());
77-
fieldExtract->replaceUsesWith(fieldAddr);
78-
_addToWorkList(fieldAddr);
79-
return;
91+
if (isUseBaseAddrOperand(use, userInst))
92+
{
93+
// Transform IRFieldExtract(IRLoad(ptr), x)
94+
// ==>
95+
// IRLoad(IRFieldAddr(ptr), x)
96+
97+
auto fieldExtract = as<IRFieldExtract>(userInst);
98+
builder.setInsertBefore(fieldExtract);
99+
auto fieldAddr = builder.emitFieldAddress(
100+
loadInst->getPtr(),
101+
fieldExtract->getField());
102+
auto loadFieldAddr = as<IRLoad>(builder.emitLoad(fieldAddr));
103+
fieldExtract->replaceUsesWith(loadFieldAddr);
104+
fieldExtract->removeAndDeallocate();
105+
106+
workList.add(loadFieldAddr);
107+
useTranslated = true;
108+
}
109+
break;
80110
}
111+
81112
case kIROp_GetElement:
82113
{
83-
// Transform the IRGetElement into a IRGetElementPtr
84-
if (!isUseBaseAddrOperand(use, user))
85-
break;
86-
auto getElement = as<IRGetElement>(user);
87-
builder.setInsertBefore(getElement);
88-
auto elemAddr = builder.emitElementAddress(
89-
getElement->getBase(),
90-
getElement->getIndex());
91-
getElement->replaceUsesWith(elemAddr);
92-
_addToWorkList(elemAddr);
93-
return;
114+
if (isUseBaseAddrOperand(use, userInst))
115+
{
116+
// Transform IRGetElement(IRLoad(ptr), x)
117+
// ==>
118+
// IRLoad(IRGetElementPtr(ptr), x)
119+
120+
auto getElement = as<IRGetElement>(userInst);
121+
builder.setInsertBefore(getElement);
122+
auto getElementPtr = builder.emitElementAddress(
123+
loadInst->getPtr(),
124+
getElement->getIndex());
125+
auto loadElementPtr = as<IRLoad>(builder.emitLoad(getElementPtr));
126+
getElement->replaceUsesWith(loadElementPtr);
127+
getElement->removeAndDeallocate();
128+
129+
workList.add(loadElementPtr);
130+
useTranslated = true;
131+
}
132+
break;
94133
}
134+
95135
case kIROp_Store:
96136
{
97137
// If the current value is being stored into a write-once temp var that
98138
// is immediately passed into a constref location in a call, we can get
99139
// rid of the temp var and replace it with `inst` directly.
100140
// (such temp var can be introduced during `updateCallSites` when we
101141
// were processing the callee.)
102-
//
103-
auto dest = as<IRStore>(user)->getPtr();
104-
if (dest->findDecorationImpl(kIROp_TempCallArgImmutableVarDecoration))
142+
143+
// Transform IRStore(storeDest, load(ptr)) where storeDest has attribute
144+
// TempCallArgImmutableVarDecoration
145+
// IRInst(storeDest)
146+
// ==>
147+
// IRInst(ptr)
148+
149+
auto storeInst = as<IRStore>(userInst);
150+
auto storeDest = storeInst->getPtr();
151+
152+
if (storeInst->getValUse() == use &&
153+
storeDest->findDecorationImpl(
154+
kIROp_TempCallArgImmutableVarDecoration))
105155
{
106-
user->removeAndDeallocate();
107-
dest->replaceUsesWith(inst);
108-
dest->removeAndDeallocate();
109-
return;
156+
storeDest->replaceUsesWith(loadInst->getPtr());
157+
userInst->removeAndDeallocate();
158+
storeDest->removeAndDeallocate();
159+
useTranslated = true;
110160
}
111161
break;
112162
}
113163
}
114-
// Insert a load before the user and replace the user with the load
115-
builder.setInsertBefore(user);
116-
auto loadInst = builder.emitLoad(inst);
117-
use->set(loadInst);
164+
165+
allUsesTranslated = allUsesTranslated && useTranslated;
118166
});
167+
168+
if (allUsesTranslated)
169+
loadInst->removeAndDeallocate();
119170
}
120171
}
121172

‎tests/bugs/gh-9073-spirv-pointer-double-dereference.slang‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
//TEST:SIMPLE(filecheck=POSITIVE): -entry main -stage closesthit -target spirv
2+
3+
// This is a minimal repro for issue 9073
4+
5+
struct Payload
6+
{
7+
uint volumeStackIndex;
8+
int volumeStack[2];
9+
10+
int GetVolume()
11+
{
12+
// This function exercises the following transformation in
13+
// transformParamsToConstRef():
14+
//
15+
// IRGetElement(
16+
// IRFieldExtract(...),
17+
// IRFieldExtract(...))
18+
//
19+
// =>
20+
//
21+
// IRGetElementPtr(
22+
// IRFieldAddress(...),
23+
// IRLoad(IRFieldAddress(...)))
24+
//
25+
// That is, that IRLoad() is correctly injected to
26+
// IRGetElementPtr() index when IRFieldExtract() is
27+
// transformed to IRFieldAddress()
28+
29+
return volumeStack[volumeStackIndex];
30+
}
31+
32+
int GetVolume2()
33+
{
34+
// This function exercises the following transformation in
35+
// transformParamsToConstRef():
36+
//
37+
// IRGetElement(
38+
// IRFieldExtract(...),
39+
// IRGetElement(...))
40+
//
41+
// =>
42+
//
43+
// IRGetElementPtr(
44+
// IRFieldAddress(...),
45+
// IRLoad(IRGetElementPtr(...)))
46+
//
47+
// That is, that IRLoad() is correctly injected to
48+
// IRGetElementPtr() index when IRGetElement() is
49+
// transformed to IRGetElementPtr()
50+
51+
return volumeStack[volumeStack[volumeStackIndex]];
52+
}
53+
}
54+
55+
RWStructuredBuffer<int> outputBuffer;
56+
57+
[shader("closesthit")]
58+
void main(inout Payload payload)
59+
{
60+
// POSITIVE-NOT: {{(error|warning).*}}:
61+
// POSITIVE: result code = 0
62+
// POSITIVE-NOT: {{(error|warning).*}}:
63+
64+
outputBuffer[0] = payload.GetVolume();
65+
66+
outputBuffer[1] = payload.GetVolume2();
67+
}

0 commit comments

Comments
 (0)