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

Add ALLOW_OPACITY_MICROMAPS template flag to RayQuery object #339

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

Conversation

amarpMSFT
Copy link
Collaborator

Inline raytracing needs to be aware OMMs may be used, just like non-inline raytracing does.

Non-inline raytracing has RAYTRACING_PIPELINE_FLAG_ALLOW_OPACITY_MICROMAPS.

But raytracing pipeline state doesn't apply to inline raytracing. So for in the inline case, add a RAYQUERY_FLAG_ALLOW_OPACITY_MICROMAPS flag to the template parameters for instantiating the RayQuery object.

Inline raytracing needs to be aware OMMs may be used, just like non-inline raytracing does.  

Non-inline raytracing has RAYTRACING_PIPELINE_FLAG_ALLOW_OPACITY_MICROMAPS.  

But raytracing pipeline state doesn't apply to inline raytracing.  So for in the inline case, add a RAYQUERY_FLAG_ALLOW_OPACITY_MICROMAPS flag to the template parameters for instantiating the RayQuery object.
@amarpMSFT amarpMSFT requested a review from damyanp October 11, 2024 17:22
@damyanp damyanp requested review from tex3d and python3kgae October 11, 2024 17:23
@damyanp damyanp added this to the Shader Model 6.9 milestone Oct 16, 2024
@damyanp
Copy link
Member

damyanp commented Oct 30, 2024

@amarpMSFT - sorry for the delay here. Does adding this new template flag to the RayQuery object mean we also need to add new diagnostics, or make any changes to DXIL? If so, then this also needs to be detailed in the proposal.

@amarpMSFT
Copy link
Collaborator Author

@amarpMSFT - sorry for the delay here. Does adding this new template flag to the RayQuery object mean we also need to add new diagnostics, or make any changes to DXIL? If so, then this also needs to be detailed in the proposal.

My guess is it would. If HLSL folks can confirm this solution is something you're ok with, the same folks might be able to quickly summarize the necessary DXIL/diag changes. But if not, I can take a stab.

@tex3d
Copy link
Collaborator

tex3d commented Oct 30, 2024

@amarpMSFT - sorry for the delay here. Does adding this new template flag to the RayQuery object mean we also need to add new diagnostics, or make any changes to DXIL? If so, then this also needs to be detailed in the proposal.

My guess is it would. If HLSL folks can confirm this solution is something you're ok with, the same folks might be able to quickly summarize the necessary DXIL/diag changes. But if not, I can take a stab.

After sorting out some questions:

  • Is SM 6.9 the only requirement, or is there an optional feature bit we need to introduce? With the DXR pipeline, we could detect feature usage from the subobject flag or use of the equivalent flag in the API. For RayQuery we don't have that luxury, so we may need an optional feature flag to indicate use in the shader.
  • Do we need additional runtime info (RDAT, PSV0) to indicate a usage with a RaytracingAccelerationStructure resource? Maybe this will be nothing because we would rely on GBV only for these checks.

This will require additions in:

  • AllocateRayQuery DXIL op
  • updated diagnostic requirements
  • validation changes
  • potential new feature info flag and/or additional runtime info (RDAT, PSV0) required for runtime checks
  • research/comment on SPIR-V alignment
  • various test additions

@llvm-beanz llvm-beanz added the Design Meeting Agenda item for the design meeting label Nov 12, 2024
@damyanp damyanp requested a review from bob80905 November 26, 2024 17:55
@amarpMSFT
Copy link
Collaborator Author

  • Is SM 6.9 the only requirement, or is there an optional feature bit we need to introduce? With the DXR pipeline, we could detect feature usage from the subobject flag or use of the equivalent flag in the API. For RayQuery we don't have that luxury, so we may need an optional feature flag to indicate use in the shader.

OMM support isn't required in SM 6.9, so a feature bit makes sense.

  • Do we need additional runtime info (RDAT, PSV0) to indicate a usage with a RaytracingAccelerationStructure resource? Maybe this will be nothing because we would rely on GBV only for these checks.

I don't think we need this. Just the feature flag is sufficient. The runtime would already have visibility into pipeline flags usage.

proposals/0024-opacity-micromaps.md Outdated Show resolved Hide resolved

The reason for separate `RAYQUERY_FLAGS` is existing `RAY_FLAGS` are
shared with non-inline raytracing ([TraceRay()][trace-ray]), where this new
flag doesn't apply, and ray flag space is a precious resource.
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 ray flag space a precious resource? How much more space is there before something bad happens?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a precious resource since it's part of the payload of every ray. Doesn't make sense to pollute this with flags that aren't actually dynamic across rays (and in this case not even applicable for non-inline rays).

wording clarification
independent of raytracing pipelines. For `RayQuery` the template for
instantiating the object includes a new optional `RAYQUERY_FLAGS` parameter:

`RayQuery<RAY_FLAGS, RAYQUERY_FLAGS>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to expand the details of this into the rest of the proposal. For instance:

  • Changes under Interchange Format Additions:
    • need a new DXIL op (new version of AllocateRayQuery) to capture the new flags argument. Decide whether to use the existing op when RAYQUERY_FLAG_NONE. My preference is that we use the old op if we do not require the new op.
    • need new RayQueryFlag enum in DXILConstants.h
  • Update Diagnostic Changes with any new relevant diagnostic cases
    • Warn about use of this flag on SM < 6.9
    • Do we need to validate that a RayQuery object uses this flag when passing the new RayFlag::ForceOMM2State (if constant)?
  • Update Validation Changes for new op and flag
  • Update Runtime Additions
    • Given this usage in RayQuery, we may require a new optional feature bit for the FeatureInfo part (since it cannot be determined by DXR state objects in the runtime).
    • After discussion, since it's benign to use the flag on a device that does not support OMM, the feature flag will not be added, and the runtime will not validate usage with feature support.
    • We will need to be clear for the intrinsic op definition that any driver supporting SM 6.9 must support the new DXIL op but may ignore the new RayQueryFlag if they do not support OMM.
  • Update Testing with implications of all these changes.

We can also capture this feedback in an issue and merge the current PR without these updates for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Meeting Agenda item for the design meeting
Projects
Status: No status
Status: Triaged
Development

Successfully merging this pull request may close these issues.

5 participants