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

wgsl: Fix field semantics even if input struct doesn't need flattening #5642

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

aleino-nv
Copy link
Collaborator

This fixes an issue where location indices of stage inputs were repeated

#5633

@aleino-nv aleino-nv requested a review from a team as a code owner November 22, 2024 11:48
@aleino-nv aleino-nv added the pr: non-breaking PRs without breaking changes label Nov 22, 2024
@aleino-nv aleino-nv force-pushed the aleino/wgsl-location branch from 3cc1f53 to 568072b Compare November 22, 2024 13:27
@aleino-nv aleino-nv force-pushed the aleino/wgsl-location branch from 568072b to 9d38a18 Compare November 22, 2024 13:37
@aleino-nv
Copy link
Collaborator Author

Falcor tests failed:

  internal/renderpasses/test_InternalPathTracer_vulkan         : FAILED (600.0 s)
    Process killed due to timeout
    View test at: http://SlangWin5:8080//unknown/internal/renderpasses/test_InternalPathTracer_vulkan

Copy link
Collaborator

@jkwak-work jkwak-work left a comment

Choose a reason for hiding this comment

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

I feel uncomfortable with this change.

The code is copied exactly from slang-ir-metal-legalize.cpp
And with this PR, WGPU side works but Metal side is failing.

I suggest to investigate why Metal is failing and fix it properly before merging this change.

@csyonghe
Copy link
Collaborator

I feel like metal needs the same treatment.

@jkwak-work
Copy link
Collaborator

I feel like metal needs the same treatment.

Yes, that must be it.

@aleino-nv aleino-nv enabled auto-merge (squash) November 25, 2024 06:02
@aleino-nv
Copy link
Collaborator Author

I feel like metal needs the same treatment.

Yes, that must be it.

@aleino-nv aleino-nv closed this Nov 25, 2024
auto-merge was automatically disabled November 25, 2024 06:04

Pull request was closed

@aleino-nv aleino-nv reopened this Nov 25, 2024
@aleino-nv aleino-nv enabled auto-merge (squash) November 25, 2024 06:05
@aleino-nv aleino-nv merged commit aaca2d2 into shader-slang:master Nov 25, 2024
15 checks passed
@aleino-nv
Copy link
Collaborator Author

I feel like metal needs the same treatment.

Here is a PR
#5666

@aleino-nv
Copy link
Collaborator Author

I feel like metal needs the same treatment.

Here is a PR #5666

Ok the PR fails, I filed #5667.
The render0 test is also broken for Metal, so it's probably best to fix that one first, and then enable this test.

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.

3 participants