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

Consolidate multiple inouts/outs into struct #6435

Merged
merged 7 commits into from
Mar 1, 2025

Conversation

mkeshavaNV
Copy link
Contributor

@mkeshavaNV mkeshavaNV commented Feb 24, 2025

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
mkeshavaNV added a commit to mkeshavaNV/slang that referenced this pull request Feb 26, 2025
@mkeshavaNV mkeshavaNV changed the title WiP consolidate outs into struct Consolidate multiple inouts/outs into struct Feb 26, 2025
mkeshavaNV added a commit to mkeshavaNV/slang that referenced this pull request Feb 26, 2025
@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

@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
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.
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

// 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

// 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

// 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

// 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
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

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.

{
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

@mkeshavaNV mkeshavaNV requested a review from csyonghe February 28, 2025 16:33
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
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants