spirv-opt: add missing OpConstantCompositeReplicateEXT support to constant manager#6616
spirv-opt: add missing OpConstantCompositeReplicateEXT support to constant manager#6616kpet wants to merge 1 commit intoKhronosGroup:mainfrom
Conversation
…stant manager Signed-off-by: Mohammadreza Ameri Mahabadian <mohammadreza.amerimahabadian@arm.com> Signed-off-by: Kevin Petit <kevin.petit@arm.com> Change-Id: I155dc7050a828d33b5f31e7b4eef8c580427ee1b
|
|
| case spv::Op::OpConstantNull: | ||
| case spv::Op::OpConstant: | ||
| case spv::Op::OpConstantComposite: | ||
| case spv::Op::OpConstantCompositeReplicateEXT: |
There was a problem hiding this comment.
I don't think this is correct. The opcode is not used when creating the constant. You will be creating a vector constant with just a single element. This will lead to problems in other areas of the code. I think the same goes for OpSpecConstantCompositeReplicateEXT.
For those opcodes, you will have to replicate the id the appropriate number of times in literal_words_or_ids.
There was a problem hiding this comment.
I did not author this change and was just upstreaming it for someone else but, diving into the code, I'm reaching the same conclusion as you. Replicating the id probably is the most straightforward solution. Another approach would be to add support for replication to CompositeConstant (and derived types) but this would be more complex/invasive I think.
| const CompositeConstant* composite = | ||
| composite_constant->AsCompositeConstant(); | ||
| ASSERT_NE(composite, nullptr); | ||
| ASSERT_FALSE(composite->GetComponents().empty()); |
There was a problem hiding this comment.
You should also assert that the number of components equals the number of elements in the vector. If not when trying to, say, fold an OpCompositeExtract that gets element 1 from the vector, the code will fail.
4 participants
No description provided.