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

cbuffer shared across modules breaks precompiled dxil support #4846

Open
cheneym2 opened this issue Aug 13, 2024 · 3 comments
Open

cbuffer shared across modules breaks precompiled dxil support #4846

cheneym2 opened this issue Aug 13, 2024 · 3 comments
Assignees
Labels
beyond-sprint Label for issues which will be worked on but not completed during a sprint. goal:forward looking Feature needed at a later date, not connected to a specific use case.

Comments

@cheneym2
Copy link
Collaborator

cheneym2 commented Aug 13, 2024

From @venkataram-nv

I found another case where slang crashes with varLayout:

file: buffers.slang

module buffers;

public cbuffer SceneConstants
{
    public float far_plane_distance;
}

file: hit.slang

import buffers;

struct RadianceHitInfo
{
    float3 contribution;
};

struct Attributes
{
    float2 bary;
};

void closesthit(inout RadianceHitInfo payload, Attributes attrib)
{
    RayDesc ray;
    ray.TMin = 0.0f;
    ray.TMax = far_plane_distance;
}
@cheneym2 cheneym2 added the goal:forward looking Feature needed at a later date, not connected to a specific use case. label Aug 13, 2024
@cheneym2 cheneym2 added this to the Q3 2024 (Summer) milestone Aug 13, 2024
@cheneym2 cheneym2 self-assigned this Aug 13, 2024
@cheneym2
Copy link
Collaborator Author

According to Yong in offline discussion, globals are a can of worms, implying this is at least a medium or large size task.

@cheneym2
Copy link
Collaborator Author

One consideration is whether this can be worked around with globals passed as function parameters.

@cheneym2
Copy link
Collaborator Author

Yong He
14 minutes ago
We have concluded that we won't deal with global variables previously, right?

Adam Cheney
14 minutes ago
I thought it was just deferred.

NEW

Yong He
14 minutes ago
So let's not work on this.

Yong He
13 minutes ago
spirv has higher priority.

Adam Cheney
13 minutes ago
Right, is it low priority or a limitation we're just accepting indefinitely?

Adam Cheney
12 minutes ago
If low priority, then it gets pushed to EOY.

Yong He
12 minutes ago
this particular crash though, I think is a bug we may need to fix.

Yong He
12 minutes ago
I don';t think that crash is related to downstream embedding at all.

Yong He
11 minutes ago
But supporting shared cbuffers fully with downstream embedding, no I don't think that is our current scope.

Yong He
11 minutes ago
slang should never crash, especially in the reflection API and in the front end.

Yong He
10 minutes ago
we can allow it to produce invalid dxil if cbuffer is used cross module.

Adam Cheney
10 minutes ago
It didn't really crash, it just reports an error.

Yong He
10 minutes ago
yeah, there should be no front end errors.

Yong He
10 minutes ago
This is totally valid code.

Yong He
8 minutes ago
In the mid future, we want to ban precompilation of modules that defines any global shader parameters. (edited)

Yong He
7 minutes ago
Like we can have a diagnostic when the user is trying to precompile buffers modules.

Yong He
7 minutes ago
Because bufferrs define a cbuffer that is effecitvely a global shader paraemter.

Yong He
7 minutes ago
That makes buffers ineligible for precompilation.

Yong He
5 minutes ago
An alternative is to allow use of global variables, but all modules linked together must have consistent binding/pipeline layout.

Yong He
5 minutes ago
We don't have to verify that ourselves, and we can leave it to the user.

Yong He
4 minutes ago
But I don't want us to get into this right now, before we have a working spirv path.

Adam Cheney
3 minutes ago
I'll defer it until after the SPIRV path is in place then.

Adam Cheney
2 minutes ago
It seems like it would be fairly easy to avoid precompiling modules that define public global variables.

@cheneym2 cheneym2 added the beyond-sprint Label for issues which will be worked on but not completed during a sprint. label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beyond-sprint Label for issues which will be worked on but not completed during a sprint. goal:forward looking Feature needed at a later date, not connected to a specific use case.
Projects
None yet
Development

No branches or pull requests

1 participant