WIP: spirv-opt: Demote r/w output variables to private #5672
Conversation
Fix KhronosGroup#5516 Pass: eliminate dead output stores Stores to write-only output variables are safe to eliminate; however, stores to read-write output variables may be required for functional correctness. If a read-write output variable is detected, then it will be demoted to a private variable and any associated stores will not be eliminated.
jeremy-lunarg
force-pushed
the
hayes-fix-5516
branch
from
697b628
to
55b6cb8
Compare
|
This look reasonable to me, but I did not look at it in detail. I'll let a code owner (someone from lunarg) do a detailed review. |
jeremy-lunarg
changed the title
|
Thanks @s-perron. There's one more thing I'd like to address before pushing this. |
| analysis::DefUseManager* def_use_mgr = context()->get_def_use_mgr(); | ||
|
|
||
| bool is_read = false; | ||
| def_use_mgr->ForEachUser(variable, [&is_read](Instruction* user) { |
There was a problem hiding this comment.
Does this need to iterate over every user? If not, should WhileEachUser be used instead?
|
This code does not address reads and writes of Output variables through OpAccessChains. So this pass is still broken for these cases. I suggest the following approach: if the Output OpVariable has an OpLoad either directly through the OpVariable or through an OpAccessChain, 1) create a Private OpVariable with the same type as the Output OpVariable and replace all uses of the Output OpVariable with the Private OpVariable; then 2) for each OpStore of the Private OpVariable, either directly through the OpVariable or through an OpAccessChain, insert a duplicate OpStore (and OpAccessChain if needed) using the original Output OpVariable. Then allow the original logic to process the Output variable and it's OpStores. |
Fix #5516
Pass: eliminate dead output stores
Stores to write-only output variables are safe to eliminate; however, stores to read-write output variables may be required for functional correctness. If a read-write output variable is detected, then it will be demoted to a private variable and any associated stores will not be eliminated.