Skip to content

Commit 3254970

Browse files
author
Tim Foley
authored
Fix a bug in IR use-def information (shader-slang#406)
The basic problem here is that when unlinking an `IRUse` from the linked list of uses, there were several cases where I was failing to set the `prevLink` field of the next node to match the `prevLink` field of the node being removed. That doesn't show up when walking the linked list of uses forward, but it breaks it whenever you have subsequent unlinking operations. This change fixes the bugs of that kind I could find, and also adds a debug validation method to try to avoid breaking it again. I also made more access to `IRUse` go through accessor methods rather than using fields directly, to try to avoid this kind of error. I stopped short of making anything `private`, because I tend to find that it creates more hassles than it avoids. A few other fixes along the way: - Made the `List<T>` type default-initialize elements when you resize it. I hadn't realized we weren't doing that. - Add a standalone `dumpIR(IRGlobalValue*)` so help when debugging issues.
1 parent 214a1fc commit 3254970

10 files changed

+200
-94
lines changed

‎source/core/list.h

+6
Original file line numberDiff line numberDiff line change
@@ -420,6 +420,12 @@ namespace Slang
420420
{
421421
for (UInt i = 0; i < _count; i++)
422422
newBuffer[i] = static_cast<T&&>(buffer[i]);
423+
424+
// Default-initialize the remaining elements
425+
for(UInt i = _count; i < size; i++)
426+
{
427+
new(newBuffer + i) T();
428+
}
423429
}
424430
FreeBuffer();
425431
}

‎source/slang/emit.cpp

+9-9
Original file line numberDiff line numberDiff line change
@@ -5183,7 +5183,7 @@ emitDeclImpl(decl, nullptr);
51835183
for(UInt aa = 0; aa < argCount; ++aa)
51845184
{
51855185
if(aa != 0) emit(", ");
5186-
emitIROperand(ctx, args[aa].usedValue, mode);
5186+
emitIROperand(ctx, args[aa].get(), mode);
51875187
}
51885188
emit(")");
51895189
}
@@ -5488,7 +5488,7 @@ emitDeclImpl(decl, nullptr);
54885488
for (UInt aa = 0; aa < argCount; ++aa)
54895489
{
54905490
if (aa != 0) Emit(", ");
5491-
emitIROperand(ctx, args[aa].usedValue, mode);
5491+
emitIROperand(ctx, args[aa].get(), mode);
54925492
}
54935493
Emit(")");
54945494
return;
@@ -5524,7 +5524,7 @@ emitDeclImpl(decl, nullptr);
55245524
UInt argIndex = d - '0';
55255525
SLANG_RELEASE_ASSERT((0 <= argIndex) && (argIndex < argCount));
55265526
Emit("(");
5527-
emitIROperand(ctx, args[argIndex].usedValue, mode);
5527+
emitIROperand(ctx, args[argIndex].get(), mode);
55285528
Emit(")");
55295529
}
55305530
break;
@@ -5536,8 +5536,8 @@ emitDeclImpl(decl, nullptr);
55365536
// texturing operation.
55375537
SLANG_RELEASE_ASSERT(argCount >= 2);
55385538

5539-
auto textureArg = args[0].usedValue;
5540-
auto samplerArg = args[1].usedValue;
5539+
auto textureArg = args[0].get();
5540+
auto samplerArg = args[1].get();
55415541

55425542
if (auto baseTextureType = textureArg->type->As<TextureType>())
55435543
{
@@ -5576,7 +5576,7 @@ emitDeclImpl(decl, nullptr);
55765576
//
55775577
// We are going to hack this *hard* for now.
55785578

5579-
auto textureArg = args[0].usedValue;
5579+
auto textureArg = args[0].get();
55805580
if (auto baseTextureType = textureArg->type->As<TextureType>())
55815581
{
55825582
emitGLSLTextureOrTextureSamplerType(baseTextureType, "sampler");
@@ -5602,7 +5602,7 @@ emitDeclImpl(decl, nullptr);
56025602
// shape.
56035603
SLANG_RELEASE_ASSERT(argCount >= 1);
56045604

5605-
auto textureArg = args[0].usedValue;
5605+
auto textureArg = args[0].get();
56065606
if (auto baseTextureType = textureArg->type->As<TextureType>())
56075607
{
56085608
auto elementType = baseTextureType->elementType;
@@ -6269,7 +6269,7 @@ emitDeclImpl(decl, nullptr);
62696269
break;
62706270
}
62716271

6272-
IRValue* arg = args[argIndex].usedValue;
6272+
IRValue* arg = args[argIndex].get();
62736273

62746274
emitIROperand(ctx, pp, IREmitMode::Default);
62756275
emit(" = ");
@@ -7142,7 +7142,7 @@ emitDeclImpl(decl, nullptr);
71427142

71437143
if(value->op == kIROp_specialize)
71447144
{
7145-
value = ((IRSpecialize*) value)->genericVal.usedValue;
7145+
value = ((IRSpecialize*) value)->genericVal.get();
71467146
}
71477147

71487148
if(value->op != kIROp_Func)

‎source/slang/ir-insts.h

+18-18
Original file line numberDiff line numberDiff line change
@@ -124,17 +124,17 @@ struct IRFieldExtract : IRInst
124124
IRUse base;
125125
IRUse field;
126126

127-
IRValue* getBase() { return base.usedValue; }
128-
IRValue* getField() { return field.usedValue; }
127+
IRValue* getBase() { return base.get(); }
128+
IRValue* getField() { return field.get(); }
129129
};
130130

131131
struct IRFieldAddress : IRInst
132132
{
133133
IRUse base;
134134
IRUse field;
135135

136-
IRValue* getBase() { return base.usedValue; }
137-
IRValue* getField() { return field.usedValue; }
136+
IRValue* getBase() { return base.get(); }
137+
IRValue* getField() { return field.get(); }
138138
};
139139

140140
// Terminators
@@ -146,7 +146,7 @@ struct IRReturnVal : IRReturn
146146
{
147147
IRUse val;
148148

149-
IRValue* getVal() { return val.usedValue; }
149+
IRValue* getVal() { return val.get(); }
150150
};
151151

152152
struct IRReturnVoid : IRReturn
@@ -168,7 +168,7 @@ struct IRUnconditionalBranch : IRTerminatorInst
168168
{
169169
IRUse block;
170170

171-
IRBlock* getTargetBlock() { return (IRBlock*)block.usedValue; }
171+
IRBlock* getTargetBlock() { return (IRBlock*)block.get(); }
172172
};
173173

174174
// Special cases of unconditional branch, to handle
@@ -191,8 +191,8 @@ struct IRLoop : IRUnconditionalBranch
191191
// on a `continue`.
192192
IRUse continueBlock;
193193

194-
IRBlock* getBreakBlock() { return (IRBlock*)breakBlock.usedValue; }
195-
IRBlock* getContinueBlock() { return (IRBlock*)continueBlock.usedValue; }
194+
IRBlock* getBreakBlock() { return (IRBlock*)breakBlock.get(); }
195+
IRBlock* getContinueBlock() { return (IRBlock*)continueBlock.get(); }
196196
};
197197

198198
struct IRConditionalBranch : IRTerminatorInst
@@ -201,9 +201,9 @@ struct IRConditionalBranch : IRTerminatorInst
201201
IRUse trueBlock;
202202
IRUse falseBlock;
203203

204-
IRValue* getCondition() { return condition.usedValue; }
205-
IRBlock* getTrueBlock() { return (IRBlock*)trueBlock.usedValue; }
206-
IRBlock* getFalseBlock() { return (IRBlock*)falseBlock.usedValue; }
204+
IRValue* getCondition() { return condition.get(); }
205+
IRBlock* getTrueBlock() { return (IRBlock*)trueBlock.get(); }
206+
IRBlock* getFalseBlock() { return (IRBlock*)falseBlock.get(); }
207207
};
208208

209209
// A conditional branch that represent the test inside a loop
@@ -230,7 +230,7 @@ struct IRIfElse : IRConditionalBranch
230230
{
231231
IRUse afterBlock;
232232

233-
IRBlock* getAfterBlock() { return (IRBlock*)afterBlock.usedValue; }
233+
IRBlock* getAfterBlock() { return (IRBlock*)afterBlock.get(); }
234234
};
235235

236236
// A multi-way branch that represents a source-level `switch`
@@ -240,9 +240,9 @@ struct IRSwitch : IRTerminatorInst
240240
IRUse breakLabel;
241241
IRUse defaultLabel;
242242

243-
IRValue* getCondition() { return condition.usedValue; }
244-
IRBlock* getBreakLabel() { return (IRBlock*) breakLabel.usedValue; }
245-
IRBlock* getDefaultLabel() { return (IRBlock*) defaultLabel.usedValue; }
243+
IRValue* getCondition() { return condition.get(); }
244+
IRBlock* getBreakLabel() { return (IRBlock*) breakLabel.get(); }
245+
IRBlock* getDefaultLabel() { return (IRBlock*) defaultLabel.get(); }
246246

247247
// remaining args are: caseVal, caseLabel, ...
248248

@@ -255,7 +255,7 @@ struct IRSwizzle : IRReturn
255255
{
256256
IRUse base;
257257

258-
IRValue* getBase() { return base.usedValue; }
258+
IRValue* getBase() { return base.get(); }
259259
UInt getElementCount()
260260
{
261261
return getArgCount() - 1;
@@ -271,8 +271,8 @@ struct IRSwizzleSet : IRReturn
271271
IRUse base;
272272
IRUse source;
273273

274-
IRValue* getBase() { return base.usedValue; }
275-
IRValue* getSource() { return source.usedValue; }
274+
IRValue* getBase() { return base.get(); }
275+
IRValue* getSource() { return source.get(); }
276276
UInt getElementCount()
277277
{
278278
return getArgCount() - 2;

‎source/slang/ir-legalize-types.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ static LegalVal legalizeCall(
232232

233233
return LegalVal::simple(context->builder->emitCallInst(
234234
callInst->type,
235-
callInst->func.usedValue,
235+
callInst->func.get(),
236236
instArgs.Count(),
237237
instArgs.Buffer()));
238238
}

‎source/slang/ir-ssa.cpp

+10-9
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ bool isPromotableVar(
110110

111111
for (auto u = var->firstUse; u; u = u->nextUse)
112112
{
113-
auto user = u->user;
113+
auto user = u->getUser();
114114
switch (user->op)
115115
{
116116
default:
@@ -231,7 +231,7 @@ IRValue* tryRemoveTrivialPhi(
231231
IRValue* same = nullptr;
232232
for (auto u : phiInfo->operands)
233233
{
234-
auto usedVal = u.usedValue;
234+
auto usedVal = u.get();
235235
assert(usedVal);
236236

237237
if (usedVal == same || usedVal == phi)
@@ -542,8 +542,8 @@ void processBlock(
542542
case kIROp_Store:
543543
{
544544
auto storeInst = (IRStore*)ii;
545-
auto ptrArg = storeInst->ptr.usedValue;
546-
auto valArg = storeInst->val.usedValue;
545+
auto ptrArg = storeInst->ptr.get();
546+
auto valArg = storeInst->val.get();
547547

548548
if (auto var = asPromotableVar(context, ptrArg))
549549
{
@@ -563,7 +563,7 @@ void processBlock(
563563
case kIROp_Load:
564564
{
565565
IRLoad* loadInst = (IRLoad*)ii;
566-
auto ptrArg = loadInst->ptr.usedValue;
566+
auto ptrArg = loadInst->ptr.get();
567567

568568
if (auto var = asPromotableVar(context, ptrArg))
569569
{
@@ -669,10 +669,10 @@ static void breakCriticalEdges(
669669

670670
for (auto edgeUse : criticalEdges)
671671
{
672-
auto pred = (IRBlock*) edgeUse->user->parent;
672+
auto pred = (IRBlock*) edgeUse->getUser()->parent;
673673
assert(pred->op == kIROp_Block);
674674

675-
auto succ = (IRBlock*)edgeUse->usedValue;
675+
auto succ = (IRBlock*)edgeUse->get();
676676
assert(succ->op == kIROp_Block);
677677

678678
IRBuilder builder;
@@ -683,6 +683,8 @@ static void breakCriticalEdges(
683683
// Create a new block that will sit "along" the edge
684684
IRBlock* edgeBlock = builder.createBlock();
685685

686+
edgeUse->debugValidate();
687+
686688
// The predecessor block should now branch to
687689
// the edge block.
688690
edgeUse->set(edgeBlock);
@@ -709,7 +711,6 @@ void constructSSA(ConstructSSAContext* context)
709711
// because our representation of SSA form doesn't allow for them.
710712
breakCriticalEdges(context);
711713

712-
713714
// Figure out what variables we can promote to
714715
// SSA temporaries.
715716
identifyPromotableVars(context);
@@ -766,7 +767,7 @@ void constructSSA(ConstructSSAContext* context)
766767
UInt predIndex = predCounter++;
767768
auto predInfo = *context->blockInfos.TryGetValue(pp);
768769

769-
IRValue* operandVal = phiInfo->operands[predIndex].usedValue;
770+
IRValue* operandVal = phiInfo->operands[predIndex].get();
770771

771772
phiInfo->operands[predIndex].clear();
772773

0 commit comments

Comments
 (0)