Shader Execution Reordering (SER) proposal

[rasmusnv]

Add Shader Execution Reordering (SER) proposal for consideration.

[damyanp]

Looks ready to take into the repo for further review. Before completing the PR, please pick the next free proposal number (at time of writing this comment, that would be 0021) and rename the file and the reference to it on line 4 appropriately.

[rdrabins]

We are interested in adding explicit communication from application in terms of what it wants (or needs) from the TraceRay call regarding reordering. I would assume we have to pass extra values to the call through extra args or through call attributes/decorations (similar to [unroll]) instead of relying on implicit behavior (which differs between vendors - some do implicitly sort/reorder, some don't).

This could be implemented using three possible values (using attribute form of expressing it in examples):

1. PreferNoReorder - the default value with the behavior according to the current spirit. Application doesn't need the reordering happen but the call still is a reorder point and implementation can decide to reorder. Note: This could be also simply spelled out in the spec to be the default if Reorder calls are present.
2. Reorder - with this value, it is equivalent to:

   HitObject hit = [PreferNoReorder]HitObject::TraceRay( ... );
   ReorderThread( hit ); 

   the value should be limited to shader stages supporting ReorderThread(...).
3. NoReorder - the value means that implementation is required to not reorder. This may be necessary for algorithm correctness in some cases (like, a wave remaining the same before and after the TraceRay call). Note: There should be a note mentioning that this can degrade performance and thus should be only used for correctness, otherwise see PreferNoReorder.

[Flakebi]

What exactly would such a NoReorder enforce? Does it require the threads doing TraceRay to stay in the same wave as threads not doing TraceRay (similar to DXR 1.1)?

Example:

void RayGen() {
  int A = WaveActiveBallot(true);
  if (<divergent cond>) {
    int C = WaveActiveBallot(true);
    [NoReorder]
    TraceRay(…);
    int D = WaveActiveBallot(true);
    // D is guaranteed to be equal to C (and made up of the same threads)
  }
  int B = WaveActiveBallot(true);
  // Is B guaranteed to be equal to A (and made up of the same threads)?
}

The second one (A==B) is problematic if the driver uses continuation-passing style to implement raytracing (e.g. https://youtu.be/KDXOAwPVTWY?t=607).

[rasmusnv]

The proposed [Reorder] is equivalent to TraceRay followed by ReorderThread. We believe this could introduce confusion to an already complex API, especially regarding when and where reordering occurs. Instead, the implementation can reasonably perform straightforward analysis to recognize sequences like TraceRay/ReorderThread/Invoke, reducing the need for additional API complexity.

Regarding [NoReorder] we agree with @Flakebi about the feasibility of such an option. While RayQuery can be implemented like this (as you mention in a separate comment) it is not certain that any implementation can support calling anyhit and intersection shaders while not modifying active threads.

We can explicitly make HitObject::TraceRay and Invoke not reorder. Then the app can control reordering by adding or omitting ReorderThreads. If the app wants the implementation to decide, legacy TraceRay can be used. Note that this does not actually guarantee anything about active threads before/after the call. It is more about the trade-off between overhead and coherence.

[Anteru]

So you're saying that some implementations can support ray query, but can't execute the shaders per lane without a resort? That's an interesting limitation which is currently not expressed in the specification, as there seem to be implied uses where the thread assignment gets not reordered by TraceRay even in the presence of any hit shaders being called by TraceRay. For example, if I want to keep quads together, you're saying there's no way to do that right now with TraceRay as explained in the SER specification?

My assumption is that any implementation support ray-query can maintain the thread assignment.

The complication in the "straightforward analysis" is that tooling may move instructions around between TraceRay, ReorderThread, Invoke, and some implementations may prefer to use the ReorderThread into either TraceRay or Invoke, and then it's undecidable which one to do if there's a ReorderThread in the middle of it, even though the implementation may know that re-ordering in TraceRay for example will provide no benefit - and it can't communicate this. ReorderThread is a fine API if all re-ordering would be limited to just that API call, but the reality is that there are multiple re-order points, and the specification language for SER specifically uses comments to explain where no reordering should happen - which is unfortunately not something the compiler knows about. I.e. in samples like:

// Invoke unified surface shading. We are already hit-coherent so not worth
// reordering again.
HitObject::Invoke( hit, payload );

[rasmusnv]

So you're saying that some implementations can support ray query, but can't execute the shaders per lane without a resort? That's an interesting limitation which is currently not expressed in the specification, as there seem to be implied uses where the thread assignment gets not reordered by TraceRay even in the presence of any hit shaders being called by TraceRay. For example, if I want to keep quads together, you're saying there's no way to do that right now with TraceRay as explained in the SER specification?

TraceRay is a reorder point so there are no guarantees about active threads afterwards. This doesn't necessarily imply that a full reorder will happen, but some amount of changes in active threads may still happen. As mentioned, RayQuery does not invoke shaders, so it is not exactly comparable to TraceRay that itself calls anyhit and intersection shaders.

The complication in the "straightforward analysis" is that tooling may move instructions around between TraceRay, ReorderThread, Invoke

Could you clarify what the tooling is here? If you mean dxc I believe we can restrict movement of things like resource accesses.

some implementations may prefer to [f]use the ReorderThread into either TraceRay or Invoke, and then it's undecidable which one to do if there's a ReorderThread in the middle of it, even though the implementation may know that re-ordering in TraceRay for example will provide no benefit - and it can't communicate this

Could you expand on this? If the driver knows that fusing with TraceRay will provide no benefit it seems logical that it should fuse with Invoke.

ReorderThread is a fine API if all re-ordering would be limited to just that API call, but the reality is that there are multiple re-order points, and the specification language for SER specifically uses comments to explain where no reordering should happen - which is unfortunately not something the compiler knows about. I.e. in samples like:

Since there is no ReorderThread call before the Invoke, it makes sense to assume that no extra effort should be made to reorder the Invoke, beyond what is required for the underlying architecture. If the underlying architecture requires an extra reorder for the Invoke anyway, it may want to utilize information provided in the most recent ReorderThread call.

[rasmusnv]

Our proposed solution is to specify that HitObject::TraceRay and HitObject::Invoke will not perform heavyweight reordering. However, they still serve as reorder points, giving the driver flexibility in how execution is scheduled. Shaders that rely on having the same sets of active threads before and after these calls are not valid.

In your example, this means that the first incoherent calculation is assumed to not be worth reordering. The only reordering that may occur is related to the efficiency of the HitObject::TraceRay operation itself, which is left to the driver’s discretion. Shader code that benefits from and receive coherence is, by definition, the code that follows ReorderThread. It is then up to the driver to decide whether it is beneficial to fuse ReorderThread with Invoke, depending on the intervening code. My assumption is that it’s better not to fuse them if there is non-uniform resource access in between, as this makes it easier for the application to predict behavior based on code changes.

Regarding tooling, it is important to ensure that non-uniform resource access remains aligned with ReorderThread. Moving accesses across it will defeat its purpose.

[Anteru]

That requires us to specify it though: "TraceRay and Invoke will not perform 'heavy-weight' reordering", I assume only in the presence of ReorderThreads, and thus behave as-if we always specified the [NoReorder] flag on them (well, nearly, because you're saying not heavy-weight, which would have to be defined as well - what does heavyweight mean?) I think it's clear by this point that we're back to the original reason we wanted this expressed clearly and explicitly, to not rely on interpretation of what heavyweight means or not.

Which brings me back to the question I raised - are there implementations which simply cannot guarantee that TraceRay maintains a 1:1 mapping? If so, that makes [NoReorder] impossible to implement, but I don't see why that should be the case given you can always fall back to inlining all shaders and using RayQuery, and most likely applications would only use that in cases where there is no reason to reorder to begin with (i.e. only one any hit shader.)

[rasmusnv]

That requires us to specify it though

Yes, we can specify this clearly and it can be phrased in a better way. The important part is what the app conveys to the driver and what the driver is allowed to do in return. The app does not explicitly expect any reordering for HitObject::TraceRay and HitObject::Invoke but the driver is still allowed flexibility for scheduling the work efficiently. The driver can account for things like the number of shaders and how to run them, and whether to fuse ReorderThread with another call (which effectively moves the reorder point).

Which brings me back to the question I raised - are there implementations which simply cannot guarantee that TraceRay maintains a 1:1 mapping?

Forcing 1:1 mapping is very restrictive and not something we'd be comfortable with. It is certainly not always the case today. It is also unclear why this would be useful given that the app has little control over thread arrangement to begin with.

Inlining is not generally a valid solution as shaders are compiled separately and pipeline creation should not consume excessive time for recompilation. It can also become problematic if there are many call sites that need inlining of the same shader(s).

[jasilvanus]

Regarding a strict [NoReorder], agreed that we shouldn't allow the app to force a 1:1 thread mapping.

The driver can't always inline everything (consider a pipeline with many shaders and many TraceRay calls in nested CHS/Miss, and a large recursion limit, or early compilation of collections), and without inlining such a guarantee would put an unreasonable burden on the implementation.
Note that this issue is not just about the set of active threads immediately before/after a TraceRay, but also about reconvergence with other threads that did not execute the TraceRay, see the earlier example by @Flakebi.

However, a [PreferNoReorder] hint on reorder points that tells the driver it shouldn't spend any additonal effort on reordering for coherence would be fine in that regard.

[rasmusnv]

The Reorder points section has been updated with a more detailed account for expected reordering behavior. Let me know if this aligns with our discussion or if there is still a need for attributes.

[rdrabins]

Actually we would like to have additional functions (also see my comment to TraceRay), one example would be:

template<payload_t> 
static HitObject HitObject::TraceRayReorderInvoke()

that would be quivalent to:

    HitObject hit = [PreferNoReorder]HitObject::TraceRay( ..., payload );
    ReorderThread( hit );
    HitObject::Invoke( hit, payload );

It'd be back to original DXR 1.0 TraceRay spirit, this time we'd now that at least on implementations that support reorder it should bring reordering here as well.

Motivations:

• some vendors can go to execution of a shader without getting back to shader.
  It has some adventages like whole wave doesn't wait for all rays to finish. This is getting increasingly important as traversal steps start to vary more and more in complex scenes.
• we'd like to encourage writing it in a single instruction instead of relying on the compiler to fuse three separate operations together. For example:

     ...
     HitObject hit = [PreferNoReorder]HitObject::TraceRay( ..., payload );
     color = tex.Sample(sampler, UV);
     ReorderThread(hit);
     HitObject::Invoke( hit, payload );

  In this case, the app decided to put the .Sample() between TraceRay and Reorder to hide the latency of the TraceRay call. Our compiler will have a hard time deciding if it's ok to move the .Sample() after the re-order or not. It's much simpler if this code is written as:

  HitObject::TraceRayReorderInvoke(payload)
  color = tex.Sample(sampler, UV);

[rasmusnv]

TraceRayReorderInvoke is similar in spirit to the attributes.

The first point is not clear to us. It is already the case that the whole wave doesn't have to wait for all rays to finish at any reorder point.

We are not quite sure why the compiler is not able to move the Sample call after ReorderThread. There are no side effects in ReorderThread and TraceRay was already a reorder point, meaning that restrictions on memory visibility apply already. A sampled texture is also read-only and is not affected by side-effects.

Our stance is that it should be easy for an implementation to merge 2-3 calls into one if it is deemed beneficial, without adding further API surface. Any shader code in between the calls is typically not affected by any side effects that may occur in TraceRay or Invoke. The only exception would be RW UAVs that are globally coherent and are accessed in between the calls. However, it is always possible to move code across ReorderThread since it does not cause or observe side effects.

[rdrabins]

Like in comment for TraceRay, we are interested in adding more communication about reordering.
We'd consider set of values:

1. PreferNoReorder - (same as for TraceRay) the default value with the behavior according to the current spirit. Application doesn't need the reordering happen but the call still is a reorder point and implementation can decide to reorder. Note: This could be also simply spelled out in the spec to be the default if Reorder calls are present.
2. Reorder - with this value, the [Reorder]HitObject::Invoke( hit, payload ) call would be equivalent to:

   ReorderThread( hit ); 
   HitObject::Invoke( hit, payload );

the value should be limited to shader stages supporting ReorderThread(...).

2. Reorder(hint,bits) - calling [Reorder(hint,bits)]HitObject::Invoke( hit, payload ) would be equivalent to:

   ReorderThread( hit, hint, bits); 
   [PreferNoReorder]HitObject::Invoke( hit, payload );

   Like above the value should be limited to shader stages supporting ReorderThread(...).

We don't propose NoReorder here. In case of traceRay, implementation already has to have the inlined raytracing in place so they can probably use it for implementing that.

The shader invocation depending on implementation may necessary involve reordering as some kind of function call implementation byproduct.
Such implementation has to deal with difficult choices in compilation. e.g.:

   ReorderThread(hit, hint, bits);
   color = tex.Sample(sampler, UV);
   HitObject::Invoke( hit, payload );

• Is sampler benefiting the reordering before it or not?
• Should I reorder twice (at reorder and at invoke) or nop at ReorderThread and reorder according to hint while Invoking the shader?

when separation of ReorderThread isn't important part of algorithm, It's easier if application writes it like that:

   color = tex.Sample(sampler, UV);
   [Reorder(hint, bits)]HitObject::Invoke( hit, payload );

[rasmusnv]

Again, we do not see a real need for this. If attributes are present at these calls, there is a risk that the user would simply specify [Reorder] on both TraceRay and Invoke, which does not actually mean what we want.

For questions like if Sample benefits from a particular location w.r.t. reordering, the assumption should be that it has an impact and that the application performs the access in the coherence scope where it makes the most sense. However, it does not necessarily hinder fusion. If it is without dependencies, it can still be moved before or after the fused call.

[pow2clk]

I'll disclose that I'm not a DXR expert. A lot of my feedback is copy editing, trying to make the document easier to read. I feel a bit stronger about my thoughts on testability and DXIL op feedback.

[pow2clk]

It's not clear what "This separation" refers to here. Is it the "Separately, HitObject" separation? I think it's the "decouple traversal" separation, I was very confused reading this initially and even having read the whole spec, I'm still not sure.

[rasmusnv]

Hopefully more clear now.

[pow2clk]

Names are all capitalized here, but lowercase below. I think they should be lowercase.

[rasmusnv]

The distinction is between opcode and intrinsic name. In the DXIL spec, opcodes are capitalized but not the intrinsic names. Does that make sense?

[pow2clk]

as above, I think HitObject should be hitObject.

[rasmusnv]

Same remark here about opcode vs intrinsic name.

[pow2clk]

I think it might be clearer to leave these out of the table above and give them their own special section. It already looks like a new topic because of the table. I think dropping them to the bottom instead of in the middle of the list of intrinsics would be less confusing.

[rasmusnv]

I moved them last, but I think I disagree about leaving them out of the initial opcode table. That table is supposed to list all opcodes and their values. The later table doesn't have the values.

[pow2clk]

This is out of order with the table above

[rasmusnv]

Moved.

[pow2clk]

For all of these, it would be useful to document any parameter restrictions that you'd like the validator to check.

[rasmusnv]

Done.

[Flakebi]

Requiring global memory coherence can be a large hit to performance. Especially since a driver can decide not to sort if the workload is coherent (or sort in a more local manner).
Using globallycoherent and DeviceMemoryBarrier makes it impossible for a driver to omit the synchronization work when it’s not needed.

I propose that we add a new UAV attribute and barrier scope that relates the synchronization to reordering. So,

• The UAV must be declared [reordercoherent]
• The UAV writer must issue a Barrier(UAV_MEMORY, THREAD_REORDER_SCOPE) between the write and the
  reorder point.

That way, when a driver decides not to reorder, the barrier can be a no-op.

[rasmusnv]

This seems useful at first glance, but expanding the memory model may overly broaden the scope of this proposal. It might be better suited as a separate proposal.

It is also unclear what use case [reordercoherent] is addressing. A UAV marked as [reordercoherent] can only communicate within the same DispatchRaysIndex, across any reordering. This essentially makes it a local scratch pad, which could be more efficiently handled using live state in the shader program (or as payload values if communicated with other shaders).

[Flakebi]

This seems useful at first glance, but expanding the memory model may overly broaden the scope of this proposal. It might be better suited as a separate proposal.

It needs one more enum value in SEMANTIC_FLAG and (optionally? see below) a [reordercoherent] marker on UAVs.

Vulkan also has a separate scope ShaderCallKHR for this.

Requiring global coherence is surely fine for some implementations but may incur a performance hit in other implementations.

It is also unclear what use case [reordercoherent] is addressing. A UAV marked as [reordercoherent] can only communicate within the same DispatchRaysIndex, across any reordering.

The marker was meant to connect the barrier to certain UAVs. This allows UAVs that are not explicitly marked to skip synchronization.
I suppose an alternative is to not require marking UAVs but expect all bound UAVs to be synchronized with a reorder-scope barrier.
Although this may be expensive if reordering happens globally. Then this would be equivalent to mark all UAVs as [globallycoherent].

[rasmusnv]

The marker was meant to connect the barrier to certain UAVs. This allows UAVs that are not explicitly marked to skip synchronization.

This part I understand. What I would like to understand is the use case for a reorder-scope barrier. Since it can only be used to synchronize within the same DispatchRaysIndex, the data that is written/read to/from the reorder-scope coherent UAV can instead be stored as live state or in payload during intermediate steps and then be written once at the end (if needed). This seems more efficient overall.

[rasmusnv]

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

[damyanp]

It looks like there's a large number of unresolved conversations on this PR - these either need to be moved into issues or addressed in the spec before we can merge it.

[rasmusnv]

I have picked proposal number 0025. All pending changes have been made. If anything is left to discuss or change I suggest we open separate issues after the merge.

[damyanp]

I have picked proposal number 0025. All pending changes have been made. If anything is left to discuss or change I suggest we open separate issues after the merge.

This seems like a good approach to me, it's getting quite hard to keep track of everything in this PR! Thank you to everyone for all the detailed comments and responses!

I'm going to mark this as approved, but I think we'll still need to get all the conversations marked as resolved before the PR can be merged, and this repo is set to require two approvals as well.

For resolving the conversations we should follow up with the original authors and/or file issues (and link them in the conversation before resolving it) if necessary.