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

Shader Execution Reordering implementation #7094

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

simoll
Copy link

@simoll simoll commented Jan 28, 2025

Shader Execution Reordering (SER) implementation

Specification PR: microsoft/hlsl-specs#277

Specification PR: microsoft/hlsl-specs#277

- This enables SER with SM6.9+
- Excludes reordercoherent and REORDER_SCOPE. Their implementation is largely orthogonal and factored into another commit.
- Includes a prototype header to inform a header-based implementation of the HitObject API in the HLSL implementation for Clang (docs/HitObject.h).
@simoll simoll requested a review from a team as a code owner January 28, 2025 11:20
Copy link
Contributor

github-actions bot commented Jan 28, 2025

✅ With the latest revision this PR passed the Python code formatter.

Copy link
Contributor

github-actions bot commented Jan 28, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@simoll
Copy link
Author

simoll commented Jan 28, 2025

@microsoft-github-policy-service agree company="NVIDIA"

"HitObject_TraceRay,HitObject_FromRayQuery,HitObject_FromRayQueryWithAttrs,HitObject_MakeMiss,HitObject_MakeNop,HitObject_Invoke,ReorderThread,HitObject_IsMiss,HitObject_IsHit,HitObject_IsNop,HitObject_RayFlags,HitObject_RayTMin,HitObject_RayTCurrent,HitObject_WorldRayOrigin,HitObject_WorldRayDirection,HitObject_ObjectRayOrigin,HitObject_ObjectRayDirection,HitObject_ObjectToWorld3x4,HitObject_ObjectToWorld4x3,HitObject_WorldToObject3x4,HitObject_WorldToObject4x3,HitObject_GeometryIndex,HitObject_InstanceIndex,HitObject_InstanceID,HitObject_PrimitiveIndex,HitObject_HitKind,HitObject_ShaderTableIndex,HitObject_SetShaderTableIndex,HitObject_LoadLocalRootTableConstant,HitObject_Attributes"
).split(","):
self.name_idx[i].category = "Shader Execution Reordering"
self.name_idx[i].shader_model = 6, 8
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this should be 6, 9? @bob80905 might be able to comment on the right way to do this here?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it should be just that simple for the most part. There may be places in here that need to change explicit checks for certain SER behavior, but as for the intrinsics, it's a one character change.

Copy link
Author

Choose a reason for hiding this comment

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

This is fixed with the latest commit

@pow2clk
Copy link
Member

pow2clk commented Jan 28, 2025

Hi @simoll!

Thanks for this! It's clearly a lot of work. That is why I have to make a request. Could you divide this up a bit into separate PRs? If that is complicated by non-technical reasons, even having separate commits that are meant to be reviewed independently would also be useful.

Some suggested divisions:

  • A change to add the new objects
  • A change to add one method (any method really) to the objects to establish that infrastructure
  • A change to add the rest of the methods
  • A change to add non-method intrinsics (ReorderThread)

Any other division that is convenient for or makes sense to you would be great as well. It's just a matter of consuming such a volume of code with so many interdependencies.

I also see that there are no checks for shader model version. I would expect to see at least a few checks for IsSM69Plus to prevent declaring the new objects in older shader models etc. I haven't gotten deep enough into the change to say exactly where, but we want to limit this feature to 6.9.

@simoll
Copy link
Author

simoll commented Jan 29, 2025

@pow2clk,

Thanks for this! It's clearly a lot of work. That is why I have to make a request. Could you divide this up a bit into separate PRs?

Yes. I've divided the change in this PR into three patches:

  1. HLSL HitObject type and default construction
  2. Remaining HLSL intrinsics
  3. reodercoherent and REORDER_SCOPE

The first PR (#7097) contains most of the novelties for DXC (static member functions for builtins, HitObject scalar type).

I suggest we keep this PR as an on-going record of the changes and open separate PRs for the patches.

I also see that there are no checks for shader model version. I would expect to see at least a few checks for IsSM69Plus to prevent declaring the new objects in older shader models etc. I haven't gotten deep enough into the change to say exactly where, but we want to limit this feature to 6.9.

There were checks: The EnableShaderExecutionReordering option is only set for SM6.9+
That said, I've made changes to only enable SER types and intrinsics in SM6.9+. This means you can have a 'class HitObject' and a function 'ReorderThread(..)' in pre-SM6.9 user code.

When specifying more than four stages in a PAQ, the
SmallVector in the PayloadAccessAnnotation starts to allocate memory.
That memory is never free'd.

The hitobject-entry-errors accidentally triggered this:

struct [raypayload] Payload
{
    float elem
-          : write(caller,closesthit,anyhit,closesthit,miss)
-          : read(caller,closesthit,anyhit,closesthit,miss);
+          : write(caller,anyhit,closesthit,miss)
+          : read(caller,anyhit,closesthit,miss);
};
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

3 participants