-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[DirectX] Add GlobalDCE pass after finalize linkage pass in DirectX backend #151071
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
base: main
Are you sure you want to change the base?
Changes from all commits
bced13d
3bca2c8
08da645
d1b498e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
; RUN: llc %s -mtriple=dxil-pc-shadermodel6.3-library --filetype=asm -o - | FileCheck %s | ||
; RUN: opt -S -passes='dxil-data-scalarization,dxil-flatten-arrays' -mtriple=dxil-unknown-shadermodel6.5-compute %s | FileCheck %s | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be better to modify the linkage of the globals in this test to be external rather than changing how we test this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @farzonl suggested changing it to opt - do you have thoughts on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah switched to opt b\c globalDCE would eliminate globals that were being used in this test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant do you have thoughts on if it would be better to modify the linkages instead of changing to opt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test case is intended to be very limited to the effects of the array flattening and vector scalarization passes. Even if we change the linkage to external it would still get cleaned up by Global DCE. Since we know which two passes are responsible for the data transformations I think its fine to just run those two passes. |
||
|
||
; Make sure we don't touch arrays without vectors and that can recurse and flatten multiple-dimension arrays of vectors | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone else looking this is the crux of the change right here. GlobalDCE is conservative so it won't remove globals marked external. This is because Linker would handle these globals and usually something like the internalizer pass would run and do a similar conversion to what we are doing here.
In HLSL's case after finalize linkage we should be able to assume that it is safe to mark all external globals with no uses as internal. If we then run GlobalDCE after this these globals will get cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're just concerned with deleting unused external variables... we could do that as part of DXILFinalizeLinkage instead of running GlobalDCE in the backend.
Are there other cases that GlobalDCE catches that we need to catch late?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Justin suggested we not do special casing of global_ctors and that GlobalDCE was a more complete solution. Hence why we closed the previous attempt at fixing this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to special case global constructors for library shaders. That's not optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use GlobalDCE we don't need to special case, things just work by converting to internal.