Skip to content

Commit

Permalink
Fix incorrect SPV stride for unsized array (#3837)
Browse files Browse the repository at this point in the history
* Fix incorrect SPV stride for unsized array (#3825)

In '-emit-spirv-directly' mode, slang generates the stride 0
for unsized array in `OpDecorate` instructions.

For unsized array, the stride is invalid, but we need to provide
a non-zero value to pass the spirv validator.

* Decorate struct with unsized array field as 'Block'

For the struct having unsized array fields, it has to be decorated
as "Block", otherwise it will fails the spirv-val.

So we add a check at in 'emitGlobalInst' when emitting spirv for
'kIROp_StructType', where if there is unsized array field inside
the struct, emit a decorate instruction for above purpose.

* Update decoration for kIROp_SizeAndAlignmentDecoration

When add a decoration node for kIROp_SizeAndAlignmentDecoration,
we implicitly convert the 64 bit size to 32 bit. In most cases, this
should not be a problem because we won't have that large data type.
However, we use 64-bit -1 to represent the size of unsized-array,
so in that case, the conversion will change the size to 0, which is
incorrect. So change that decoration to use 64-bit size.

---------

Co-authored-by: Yong He <yonghe@outlook.com>
  • Loading branch information
kaizhangNV and csyonghe authored Mar 27, 2024
1 parent b346a93 commit 5692879
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 3 deletions.
29 changes: 27 additions & 2 deletions source/slang/slang-emit-spirv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1415,12 +1415,21 @@ struct SPIRVEmitContext
if (m_decoratedSpvInsts.add(getID(resultSpvType)))
{
IRSizeAndAlignment sizeAndAlignment;
getNaturalSizeAndAlignment(m_targetProgram->getOptionSet(), ptrType->getValueType(), &sizeAndAlignment);
uint32_t stride;

getNaturalSizeAndAlignment(m_targetProgram->getOptionSet(), valueType, &sizeAndAlignment);
uint64_t valueSize = sizeAndAlignment.size;

// Any unsized data type (e.g. struct or array) will have size of kIndeterminateSize,
// in such case the stride is invalid, so we have to provide a non-zero value to pass the
// spirv validator.
stride = (valueSize >= (uint64_t)sizeAndAlignment.kIndeterminateSize) ?
0xFFFF : (uint32_t)sizeAndAlignment.getStride();
emitOpDecorateArrayStride(
getSection(SpvLogicalSectionID::Annotations),
nullptr,
resultSpvType,
SpvLiteralInteger::from32((uint32_t)sizeAndAlignment.getStride()));
SpvLiteralInteger::from32(stride));
}
}
return resultSpvType;
Expand All @@ -1431,12 +1440,28 @@ struct SPIRVEmitContext
{
List<IRType*> types;
for (auto field : static_cast<IRStructType*>(inst)->getFields())
{
types.add(field->getFieldType());
}
auto spvStructType = emitOpTypeStruct(
inst,
types
);
emitDecorations(inst, getID(spvStructType));

auto structType = as<IRStructType>(inst);
uint64_t structSize = 0;
if (auto layoutDecor = structType->findDecoration<IRSizeAndAlignmentDecoration>())
{
structSize = layoutDecor->getSize();
}

if (structSize >= (uint64_t)IRSizeAndAlignment::kIndeterminateSize)
{
IRBuilder builder(inst);
auto decoration = builder.addDecoration(inst, kIROp_SPIRVBlockDecoration);
emitDecoration(getID(spvStructType), decoration);
}
emitLayoutDecorations(as<IRStructType>(inst), getID(spvStructType));
return spvStructType;
}
Expand Down
1 change: 1 addition & 0 deletions source/slang/slang-ir-insts.h
Original file line number Diff line number Diff line change
Expand Up @@ -3323,6 +3323,7 @@ struct IRBuilder
IRBasicType* getVoidType();
IRBasicType* getBoolType();
IRBasicType* getIntType();
IRBasicType* getInt64Type();
IRBasicType* getUIntType();
IRBasicType* getUInt64Type();
IRBasicType* getCharType();
Expand Down
3 changes: 2 additions & 1 deletion source/slang/slang-ir-layout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,11 +363,12 @@ Result getSizeAndAlignment(CompilerOptionSet& optionSet, IRTypeLayoutRules* rule
IRBuilder builder(module);

auto intType = builder.getIntType();
auto int64Type = builder.getInt64Type();
builder.addDecoration(
type,
kIROp_SizeAndAlignmentDecoration,
builder.getIntValue(intType, (IRIntegerValue)rules->ruleName),
builder.getIntValue(intType, sizeAndAlignment.size),
builder.getIntValue(int64Type, sizeAndAlignment.size),
builder.getIntValue(intType, sizeAndAlignment.alignment));
}

Expand Down
5 changes: 5 additions & 0 deletions source/slang/slang-ir.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2649,6 +2649,11 @@ namespace Slang
return (IRBasicType*)getType(kIROp_IntType);
}

IRBasicType* IRBuilder::getInt64Type()
{
return (IRBasicType*)getType(kIROp_Int64Type);
}

IRBasicType* IRBuilder::getUIntType()
{
return (IRBasicType*)getType(kIROp_UIntType);
Expand Down
23 changes: 23 additions & 0 deletions tests/bugs/gh-3825.slang
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@

// Emit 0xFFFF as the stride value for the unsized array

//TEST:SIMPLE(filecheck=CHECK): -entry fragment -stage fragment -emit-spirv-directly -target spirv-assembly -emit-spirv-directly
struct Descriptors {
uint count;
uint array[];
}

struct Context {
Descriptors *descriptors;
}

[[vk::binding(0)]] ConstantBuffer<Context> context;

// Dummy entry point.
[shader("fragment")]
float4 fragment(): SV_Target
{
return float4(float(context.descriptors[0].array[0]), 1., 1., 1.);
}

// CHECK: OpDecorate %_ptr_PhysicalStorageBuffer__runtimearr_uint ArrayStride 65535

0 comments on commit 5692879

Please sign in to comment.