Drop undef/poison initializers for global variables in SPIR-V translation#3661
Drop undef/poison initializers for global variables in SPIR-V translation#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? |
lib/SPIRV/SPIRVReader.cpp
Outdated
|
|
||
| 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.
There was a problem hiding this comment.
The point is that backend omits custom "undef" initializers, because they don't make sense. Updated the patch to focus on this specific situation along with PR title & description
There was a problem hiding this comment.
@MrSidims please, take a look at the updated approach
Backend does not produce OpUndef by itself, while translator does. That creates the difference |
aobolensk
force-pushed
the
undef-to-poison
branch
from
0eb7308 to
5f0b1e8
Compare
aobolensk
changed the title
aobolensk
force-pushed
the
undef-to-poison
branch
2 times, most recently
from
0e198c7 to
fa057ab
Compare
aobolensk
force-pushed
the
undef-to-poison
branch
from
fa057ab to
248bfe8
Compare
3 participants
Align the translator with the LLVM SPIR-V backend behavior : undef/poison initializers are dropped unless the variable is constant with an aggregate type. This avoids emitting unnecessary OpUndef initializers in the generated SPIR-V.