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.

Sign up for GitHub

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

Jump to bottom

Consolidate multiple inouts/outs into struct #6435

Merged
merged 7 commits into from
Mar 1, 2025
Merged

Consolidate multiple inouts/outs into struct #6435

merged 7 commits into from
Mar 1, 2025

Conversation

mkeshavaNV
Copy link
Contributor

@mkeshavaNV mkeshavaNV commented Feb 24, 2025 •
edited
Loading

Consolidate multiple inout/outs into struct

Fixes #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.

@mkeshavaNV mkeshavaNV added the pr: non-breaking PRs without breaking changes label Feb 24, 2025
@mkeshavaNV mkeshavaNV force-pushed the bugfix/gh-5121_1 branch 7 times, most recently from 259b608 to 1b2e686 Compare February 26, 2025 07:20
@slangbot slangbot mentioned this pull request Feb 26, 2025
Merged
mkeshavaNV added a commit to mkeshavaNV/slang that referenced this pull request Feb 26, 2025
@mkeshavaNV
Merge pull request #4 from slangbot/format-6435-bugfix/gh-5121_1
4744aca 
Format code for PR shader-slang#6435
@mkeshavaNV mkeshavaNV changed the title WiP consolidate outs into struct Consolidate multiple inouts/outs into struct Feb 26, 2025
@slangbot slangbot mentioned this pull request Feb 26, 2025
Merged
mkeshavaNV added a commit to mkeshavaNV/slang that referenced this pull request Feb 26, 2025
@mkeshavaNV
Merge pull request #5 from slangbot/format-6435-bugfix/gh-5121_1
00c2ae8 
Format code for PR shader-slang#6435
@mkeshavaNV
Copy link
Contributor Author

/format

@shader-slang shader-slang deleted a comment from slangbot Feb 26, 2025
@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@slangbot slangbot mentioned this pull request Feb 26, 2025
Merged
@shader-slang shader-slang deleted a comment from slangbot Feb 26, 2025
mkeshavaNV added a commit to mkeshavaNV/slang that referenced this pull request Feb 26, 2025
@mkeshavaNV
Merge pull request #6 from slangbot/format-6435-bugfix/gh-5121_1
3e3deb7 
Format code for PR shader-slang#6435
@mkeshavaNV
Consolidate multiple inout/outs into struct
924e570 
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.
kaizhangNV
kaizhangNV reviewed Feb 26, 2025
View reviewed changes
tests/vkray/multipleinout.slang Outdated
dummy = HitTriangleVertexPosition(0);
}

// CHECK: OpCapability RayTracingKHR
Copy link
Contributor

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

Copy link
Contributor Author

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"
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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

tests/vkray/multipleinout.slang
// CHECK-DAG: OpDecorate %{{.*}} BuiltIn HitTriangleVertexPositionsKHR

// CHECK: %ReflectionRay = OpTypeStruct %v4float
// CHECK: %_struct_{{.*}} = OpTypeStruct %ReflectionRay %v3float
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

and 57

Copy link
Contributor Author

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
// 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 %{{.*}}
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@mkeshavaNV mkeshavaNV marked this pull request as ready for review February 27, 2025 06:45
@mkeshavaNV mkeshavaNV requested a review from a team as a code owner February 27, 2025 06:45
csyonghe
csyonghe reviewed Feb 28, 2025
View reviewed changes
source/slang/slang-ir-glsl-legalize.cpp Outdated
valueType = inOutType->getValueType();

auto key = builder->createStructKey();
builder->addNameHintDecoration(key, UnownedStringSlice("field"));
Copy link
Collaborator

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());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

source/slang/slang-ir-glsl-legalize.cpp Outdated
auto _paramType = _param->getDataType();
IRType* valueType = _paramType;

if (as<IROutType>(_paramType))
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

source/slang/slang-ir-glsl-legalize.cpp Outdated
{
auto firstBlock = func->getFirstBlock();

// Now we run the consolidation step, but if we've already
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@mkeshavaNV
Copy link
Contributor Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@slangbot slangbot mentioned this pull request Feb 28, 2025
Merged
@mkeshavaNV mkeshavaNV requested a review from csyonghe February 28, 2025 16:33
csyonghe
csyonghe approved these changes Mar 1, 2025
View reviewed changes
Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

looks good!

@csyonghe csyonghe merged commit b86925c into shader-slang:master Mar 1, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaizhangNV kaizhangNV kaizhangNV left review comments

@csyonghe csyonghe csyonghe approved these changes

Assignees
No one assigned
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mkeshavaNV @slangbot @csyonghe @kaizhangNV