Translate OpUndef to poison instead of undef in SPIR-V reader#3661
Translate OpUndef to poison instead of undef in SPIR-V reader#3661aobolensk wants to merge 1 commit intoKhronosGroup:mainfrom
Conversation
aobolensk
force-pushed
the
undef-to-poison
branch
from
23cce6e to
0eb7308
Compare
|
@MrSidims please, take a look |
vmaksimo
left a comment
vmaksimo
left a comment
There was a problem hiding this comment.
I seriously doubt we should make this change before LLVM totally removes undef.
We already aligned to the community wherever possible (cec12d6) in terms of using poison, but translation of OpUndef to poison does not seem reasonable to me right now.
Actually, that was done now for the purpose of alignment with SPIR-V backend in LLVM |
LLVM is not going to remove undef.
Do we understand, why SPIR-V module produced by the backend results in poison creation after reverse translation when SPIR-V module produced by the translator is resulting in undef after the round trip? |
|
|
||
| case OpUndef: | ||
| return mapValue(BV, UndefValue::get(transType(BV->getType()))); | ||
| return mapValue(BV, PoisonValue::get(transType(BV->getType()))); |
There was a problem hiding this comment.
We can't do the replacement unconditionally. If we really want to invest into this, then we should do this following https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#_validity_and_defined_behavior aka check which instruction generates/consume OpUndef and if consuming this undef/poison value is forbidden for an instruction - undef should be generated in LLVM IR, and poison otherwise.
There was a problem hiding this comment.
Fair enough. Shall we follow the logic described in the spec directly? As I can see, there could be several "forbidden consumers" such as conditional branch instructions. Or are there any other assumptions? The check seems to be very non-elegant, because it tries to evaluate the context of the users (other mappers here do not perform such checks)
There was a problem hiding this comment.
Please correct me if I'm wrong. The patch addresses this fixme, right? If so, I would like to understand, why roundtrip through llvm-spirv alone and through backend + llvm-spirv are different before suggesting an approach.
Backend does not produce OpUndef by itself, while translator does. That creates the difference |
3 participants
LLVM is migrating from undef to poison values. Update the OpUndef translation in SPIRVReader to produce PoisonValue instead of UndefValue.
Enable round-trip for
test/var_undef.llwhere the only difference between the approaches was related toundefvspoison