-
Notifications
You must be signed in to change notification settings - Fork 264
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
Consolidate multiple inouts/outs into struct #6435
Conversation
259b608
to
1b2e686
Compare
Format code for PR shader-slang#6435
4744aca
to
652e28b
Compare
Format code for PR shader-slang#6435
bef2f2a
to
bc3b6c2
Compare
/format |
🌈 Formatted, please merge the changes from this PR |
Format code for PR shader-slang#6435
3e3deb7
to
9d8ec3e
Compare
Fixes shader-slang#5121 VUID-StandaloneSpirv-IncomingRayPayloadKHR-04700 requires that there be only one IncomingRayPayloadKHR per entry point. This change does two things: 1. If an entry point has the one inout or out, or has only ins, then stay with current implementation. 2. If there are multiple outs/inouts, then create a new structure to consolidate these fields and emit this structure. These two code paths are split into two separate functions for clarity. This patch also adds a new test: multipleinout.slang to test this.
9d8ec3e
to
924e570
Compare
tests/vkray/multipleinout.slang
Outdated
dummy = HitTriangleVertexPosition(0); | ||
} | ||
|
||
// CHECK: OpCapability RayTracingKHR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check from line 28-34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
tests/vkray/multipleinout.slang
Outdated
// CHECK: OpMemoryModel Logical GLSL450 | ||
// CHECK: OpEntryPoint ClosestHitKHR %main "main" %{{.*}} %{{.*}} %gl_PrimitiveID %{{.*}} %gl_InstanceID %colors %{{.*}} | ||
|
||
// CHECK: OpName %ReflectionRay "ReflectionRay" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check 37-42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
tests/vkray/multipleinout.slang
Outdated
// CHECK: OpName %colors "colors" | ||
// CHECK: OpName %main "main" | ||
|
||
// CHECK-DAG: OpDecorate %gl_InstanceID BuiltIn InstanceId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to check 44-53
// CHECK-DAG: OpDecorate %{{.*}} BuiltIn HitTriangleVertexPositionsKHR | ||
|
||
// CHECK: %ReflectionRay = OpTypeStruct %v4float | ||
// CHECK: %_struct_{{.*}} = OpTypeStruct %ReflectionRay %v3float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can keep this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and 57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
// CHECK: OpExtension "SPV_KHR_storage_buffer_storage_class" | ||
// CHECK: OpExtension "SPV_KHR_ray_tracing_position_fetch" | ||
// CHECK: OpMemoryModel Logical GLSL450 | ||
// CHECK: OpEntryPoint ClosestHitKHR %main "main" %{{.*}} %{{.*}} %gl_PrimitiveID %{{.*}} %gl_InstanceID %colors %{{.*}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the synthesized struct in the entry point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaizhangNV The spirv code generated is like so. So it does include the entry point
OpEntryPoint ClosestHitKHR %main "main" %48 %31 %gl_PrimitiveID %25 %gl_InstanceID %colors %11
...
%11 = OpVariable %_ptr_IncomingRayPayloadKHR__struct_5 IncomingRayPayloadKHR ; Location 0
So it does cover it, but I guess this can't be added as a check condition here since the actual assembly instruction number would differ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see.
Then it's fine, just keep it as it.
valueType = inOutType->getValueType(); | ||
|
||
auto key = builder->createStructKey(); | ||
builder->addNameHintDecoration(key, UnownedStringSlice("field")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the name "field"? the name should be coming from _param
.
just do:
if (auto nameDecor = _param->findDecoration<IRNameHintDecoration>())
builder->addNameHintDecoration(key, nameDecor->getName());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
auto _paramType = _param->getDataType(); | ||
IRType* valueType = _paramType; | ||
|
||
if (as<IROutType>(_paramType)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as<IROutTypeBase>
should cover both cases in this if.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
{ | ||
auto firstBlock = func->getFirstBlock(); | ||
|
||
// Now we run the consolidation step, but if we've already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like we are doing consolidation too late. Maybe consolidation should be called directly from legalizeEntryPointForGLSL
instead of from legalizeEntryPointParameterForGLSL
so we don't need to track if a parameter has already been processed or not, and we also won't need that hasSingleOutOrInOutParam
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for that suggestion. That will indeed simplify the code. Updated.
7d9d130
to
ef9a937
Compare
/format |
🌈 Formatted, please merge the changes from this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
Consolidate multiple inout/outs into struct