Skip to content
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.

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

Check for undefined %id in spirv_asm block. #5966

Merged
merged 3 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 62 additions & 0 deletions source/slang/slang-check-expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5311,6 +5311,10 @@ Expr* SemanticsExprVisitor::visitSPIRVAsmExpr(SPIRVAsmExpr* expr)
// We will iterate over all the operands in all the insts and check
// them
bool failed = false;

// Track %id's that have been defined in this asm block.
HashSet<Name*> definedIds;

for (auto& inst : expr->insts)
{
// It's not automatically a failure to not have info, we just won't
Expand All @@ -5327,6 +5331,52 @@ Expr* SemanticsExprVisitor::visitSPIRVAsmExpr(SPIRVAsmExpr* expr)
0);
continue;
}
int resultIdIndex = -1;
if (opInfo)
{
resultIdIndex = opInfo->resultIdIndex;
}
else if (inst.opcode.flavor == SPIRVAsmOperand::TruncateMarker)
{
// If this is __truncate, register the result id in the third operand.
resultIdIndex = 1;
}
else
{
// If there is no opInfo, just register all Ids as defined.
for (auto& operand : inst.operands)
{
if (operand.flavor == SPIRVAsmOperand::Id)
{
definedIds.add(operand.token.getName());
}
}
}

// Register result ID.
if (resultIdIndex != -1)
{
if (inst.operands.getCount() <= resultIdIndex)
{
failed = true;
getSink()->diagnose(
inst.opcode.token,
Diagnostics::spirvInstructionWithNotEnoughOperands,
inst.opcode.token);
continue;
}
auto& resultIdOperand = inst.operands[resultIdIndex];

if (!definedIds.add(resultIdOperand.token.getName()))
{
failed = true;
getSink()->diagnose(
inst.opcode.token,
Diagnostics::spirvIdRedefinition,
inst.opcode.token);
continue;
}
}

const bool isLast = &inst == &expr->insts.getLast();
for (Index operandIndex = 0; operandIndex < inst.operands.getCount(); ++operandIndex)
Expand Down Expand Up @@ -5438,6 +5488,18 @@ Expr* SemanticsExprVisitor::visitSPIRVAsmExpr(SPIRVAsmExpr* expr)
}
operand.knownValue = builtinVarKind.value();
}
else if (operand.flavor == SPIRVAsmOperand::Id)
{
if (!definedIds.contains(operand.token.getName()))
{
failed = true;
getSink()->diagnose(
operand.token,
Diagnostics::spirvUndefinedId,
operand.token);
return;
}
}
if (operand.bitwiseOrWith.getCount() &&
operand.flavor != SPIRVAsmOperand::Literal &&
operand.flavor != SPIRVAsmOperand::NamedValue)
Expand Down
12 changes: 11 additions & 1 deletion source/slang/slang-diagnostic-defs.h
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,17 @@ DIAGNOSTIC(
Error,
spirvInvalidTruncate,
"__truncate has been given a source smaller than its target")

DIAGNOSTIC(29112, Error, spirvInstructionWithNotEnoughOperands, "not enough operands for $0")
DIAGNOSTIC(
29113,
Error,
spirvIdRedefinition,
"SPIRV id '%$0' is already defined in the current assembly block")
DIAGNOSTIC(
29114,
Error,
spirvUndefinedId,
"SPIRV id '%$0' is not defined in the current assembly block location")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the 'location' part there because the ID may be defined in a later location within the block?
If not then it seems clearer to leave out 'location'.
If so, then something like "...in the current assembly block, at the current location" seems slightly more clear to me.

//
// 3xxxx - Semantic analysis
//
Expand Down
10 changes: 10 additions & 0 deletions tests/diagnostics/invalid-spv-obj.slang
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//TEST:SIMPLE(filecheck=CHECK): -target spirv

[shader("compute")]
[numthreads(1,1,1)]
void main() {
int var2 = spirv_asm {
// CHECK: ([[# @LINE+1]]): error 29114
result: $$int = OpIMul $(15) %invalidId
};
}
Loading