opt: dissolve OpCopyLogical of OpCompositeConstruct in simplification#6746
opt: dissolve OpCopyLogical of OpCompositeConstruct in simplification#6746SteveUrquhart wants to merge 1 commit into
Conversation
SteveUrquhart
force-pushed
the
fold-copylogical-of-composite
branch
from
50af2e5 to
422ef27
Compare
s-perron
left a comment
s-perron
left a comment
There was a problem hiding this comment.
Generally looks good. However, we usually assume the spir-v is valid coming into the pass is valid, so there is not need to handle cases where it is not. Those can be asserts. The exception to that is if this is part of DXC legalization. DXC is generating invalid code, and we want to handle it to eventually make it valid.
| for (uint32_t i = 0; i < num_constituents; ++i) { | ||
| const uint32_t cid = src_inst->GetSingleWordInOperand(i); | ||
| Instruction* cdef = def_use_mgr->GetDef(cid); | ||
| if (cdef->type_id() == expected_type_ids[i]) { | ||
| continue; | ||
| } | ||
| Instruction* constituent_type_inst = def_use_mgr->GetDef(cdef->type_id()); | ||
| Instruction* expected_type_inst = def_use_mgr->GetDef(expected_type_ids[i]); | ||
| if (constituent_type_inst->opcode() == spv::Op::OpTypePointer || | ||
| expected_type_inst->opcode() == spv::Op::OpTypePointer) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we can remove this loop and add an assert in the next loop that type for the opcopylogical is not a pointer. If there was a pointer in the struct or array, then the original OpCopyLogical would have been invalid. I don't mind transforming invalid code into different invalid code. I don't like doing a lot of extra work to avoid doing that.
There was a problem hiding this comment.
Thanks, I hadn't thought of using assert as a tool.
There was a problem hiding this comment.
What about physical storage buffer? Could that not be a member? It has a well defined size.
There was a problem hiding this comment.
You're right, the assert was too much. We still can't fold it, so bailing is the best option, that preserves valid IR. I added a test for PSB pointers.
There was a problem hiding this comment.
I don't think that physical storage buffer makes a difference. If you have a valid OpCopyLogical, the type for the pointer must be the same in the source and target type, and we would have continued at line 2177.
I still want to know which cases reach line 2183. I believe they are invalid coming into the pass.
SteveUrquhart
force-pushed
the
fold-copylogical-of-composite
branch
from
422ef27 to
ab5f507
Compare
SteveUrquhart
force-pushed
the
fold-copylogical-of-composite
branch
from
ab5f507 to
8f3ac5d
Compare
3 participants
As described in microsoft/DirectXShaderCompiler#8382, this PR adds a new folding rule to deal with the new SPIR-V the optimizer is receiving from dxc since microsoft/DirectXShaderCompiler#7530. By rewriting
as
...we can continue to legalize the technically-invalid
...which dxc has always emitted.