Skip to content

[Draft] Rewrite output parameters #5249

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

llvm-beanz
Copy link
Collaborator

@llvm-beanz llvm-beanz commented May 30, 2023

This change is a huge swath of changes rewriting how HLSL represents pointers, references and arrays as well as the handling of output parameters in function signatures.

The main changes included in this PR are:

  • Restoring Array->Pointer Decay in the HLSL ASTs.
  • Representing all output parameters using reference types in the AST.
  • Adding a new HLSLOutParamExpr AST node for output parameters. *Adding a new HLSLArrayTemporaryExpr to represent temporary array values.

There are also a large number of associated fixes and patches to make the code generation and optimization code resilient to the new AST and IR structures that are generated.

Disclaimer: This is a massive PR with no good way to break it up. In the current state there are still some test failures that I need to resolve, but I want to get this in front of people to review before it gets any bigger.

Fixes #5377

This change is a huge swath of changes rewriting how HLSL represents
pointers, references and arrays as well as the handling of output
parameters in function signatures.

The main changes included in this PR are:
* Restoring Array->Pointer Decay in the HLSL ASTs.
* Representing all output parameters using reference types in the AST.
* Adding a new HLSLOutParamExpr AST node for output parameters.
*Adding a new HLSLArrayTemporaryExpr to represent temporary array
values.

There are also a large number of associated fixes and patches to make
the code generation and optimization code resilient to the new AST and
IR structures that are generated.
@llvm-beanz
Copy link
Collaborator Author

For more information on the changes contained here see: Revising HLSL out Parameters.pdf

@AppVeyorBot
Copy link

Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

I took a look at the SPIR-V changes, and nothing looks like it should be a problem.

@s-perron
Copy link
Collaborator

I'll take a look at the failing spir-v test to make sure the newly generated spir-v makes sense.

@s-perron
Copy link
Collaborator

Some of the failing tests are real problems that need to be fixed:

  • FileTest.BinaryOpAssignOpaqueArray - Some optimizations are not happening. They are probably unaware of OpCopyMemory. Spirv-opt will have to be improved. This has to be fixed before merging.
  • FileTest.FunctionInOutParamLocalResource - The is an error related to the counter variables. I don't know if this is intended, but I expect it is not. I'm not sure what needs to be done in this case.
  • FileTest.FunctionInOutParamIsomorphism - The optimization is not as good as it could be. Not necessary to fix, but would be nice.

I still have to look at ~30 more failing test cases.

@s-perron
Copy link
Collaborator

  • FileTest.TextureGetDimensions - I cannot tell from the test if this is a problem or not. In some cases where the output variable is not used, the temp does not get copied out to the original variable. This is one example of many. We should check what happens when the value it used to make sure the code is okay.
  • FileTest.ByteAddressBufferTemplatedLoadMatrix - The spirv is incorrect. The OpCopyMemory is trying to copy a value that has already been loaded. Affects other test cases as well.
  • FileTest.SpirvInterpolationVS - I think the test case is invalid, and needs to be updated. This is more than just updating the expected outputs.
  • FileTest.MeshShadingEXTAmplification - There is a new error message, but the error message seems wrong. It says that expected groupshared object as argument to DispatchMesh(), but the argument that it points to is groupshared.
  • FileTest.ReduceLoadSize - OpCopyMemory is trying to copy between the objects of different type. The types are different because of their decorations. Before spir-v 1.4 you have to do what the old code does: extract individual part, and reconstruct them. In spir-v 1.4, we can load, OpCopyLogical, and store.

@llvm-beanz
Copy link
Collaborator Author

Related to #2419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

out and inout should always be references
3 participants