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

Incorrect argument parsing by slangc -- partially fix #3931 #3972

Conversation

ArielG-NV
Copy link
Contributor

Problems:

  1. Incorrect slangc arg parsing (possible to have row major & column major in option_set) -- addressed in pr.
  2. spirv-opt 2023-6 & upstream incorrectly optimizes code and removes member decorations in some odd cases, works with spirv-opt 2024-1 -- not solved in this pr since we use upstream of spirv-opt currently to solve different issues.

problems:
1. incorrect slangc arg parsing (possible to have rowMajor & colMajor in option_set) -- addressed in commit
2. spirv-opt 2023-6 incorrectly optimizes code and removes member decorations in some odd cases, fixed in spirv-opt 2024-1 -- added test shader-slanggh-3931.slang to ensure we are using spirv-opt 2024
@ArielG-NV ArielG-NV changed the title incorrect argument parsing by slangc -- partially fix #3931 Incorrect argument parsing by slangc -- partially fix #3931 Apr 17, 2024
@@ -0,0 +1,24 @@
//TEST:SIMPLE(filecheck=CHECK): -O0 -target spirv -emit-spirv-directly -stage compute -entry computeMain -matrix-layout-row-major
//COM:TEST:SIMPLE(filecheck=CHECK): -O2 -target spirv -emit-spirv-directly -stage compute -entry computeMain -matrix-layout-row-major
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the second test meant to be commented out with "//COM:"?
I thought this test is to test if the row-major-ness works properly or not when "-O2" is used, isn't it?

Copy link
Contributor Author

@ArielG-NV ArielG-NV Apr 18, 2024

Choose a reason for hiding this comment

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

there were 2 problems:

Incorrect slangc arg parsing (possible to have row major & column major in option_set) -- addressed in pr.

spirv-opt 2023-6 & upstream incorrectly optimizes code and removes member decorations in some odd cases, works with spirv-opt 2024-1 -- not solved in this pr since we use upstream of spirv-opt currently to solve different issues.

Of these, we can only fix problem one since this is a problem inside Slang. This allows all code to work every-run with row major'ness set with 1 caveat which is problem 2.
Problem 2 cannot be addressed since the spirv-opt version we are using cannot be downgraded. This means that spirv optimization may produce wrong code. using O0 implies no optimization, O2 implies using spirv-opt, which we cannot do.

@csyonghe csyonghe merged commit 355a8d8 into shader-slang:master Apr 18, 2024
15 of 16 checks passed
aroidzap pushed a commit to aroidzap/slang that referenced this pull request Apr 21, 2024
djohansson pushed a commit to djohansson/slang that referenced this pull request Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants