Use poison instead of undef values wherever possible
#2960
Conversation
In reverse translation always use `poison` values except cases where we translate `OpUndef`. In other cases common sense helped to define where it is safe to replace `undef` to `poison` (e.g. in creating structions, regularization passes). This resolves KhronosGroup#2953 and aligns us with the LLVM community in terms of generating IR.
| @@ -231,7 +231,7 @@ void SPIRVRegularizeLLVMBase::buildUMulWithOverflowFunc(Function *UMulFunc) { | |||
| // umul.with.overflow intrinsic return a structure, where the first element | |||
| // is the multiplication result, and the second is an overflow bit. | |||
| auto *StructTy = UMulFunc->getReturnType(); | |||
| auto *Agg = Builder.CreateInsertValue(UndefValue::get(StructTy), Mul, {0}); | |||
| auto *Agg = Builder.CreateInsertValue(PoisonValue::get(StructTy), Mul, {0}); | |||
There was a problem hiding this comment.
Why changes in SPIRVRegularizeLLVM don't break tests? We don't have poison in SPIR-V, meanwhile you add it to the input LLVM IR.
There was a problem hiding this comment.
I'm not sure, maybe because I changed check-lines in tests as well?
But whatever the reason, I think that we can safely use poison instead of undef here because it's not the actual input, but the value we insert ourselves. This way we should not break input semantics.
What do you think?
There was a problem hiding this comment.
But whatever the reason, I think that we can safely use poison instead of undef here because it's not the actual input, but the value we insert ourselves. This way we should not break input semantics.
Incorrect. If we insert something in LLVM IR that SPIRVWriter doesn't support - it will emit an error.
So I meant to say: if the translator doesn't handle poison, then this change would result in a regression when compiling a user code. But I've double checked it, and it seems like the translator is actually handling poison values (at least at some extend). For example in transConstant for the following LLVM IR snippet:
%structtype = type { i32, float }
define spir_func %structtype @foo() {
entry:
ret %structtype { i32 1, float poison }
}
this check results in true, so OpUndef is added instead of poison.
So at least for constants we are safe. Revisiting our offline conversation: guess we can replace more 'undef' code to 'poison' during forward translation.
|
Restarting CI |
In reverse translation always use
poisonvalues except cases where we translateOpUndef. In other cases common sense was used to define where it is safe to replaceundeftopoison(e.g. in creating structures, regularization passes).This resolves #2953 and aligns us with the LLVM community in terms of generating IR.