Read old SPV_INTEL_float4 type for StochasticRound*ToE2M1INTEL#3901
Read old SPV_INTEL_float4 type for StochasticRound*ToE2M1INTEL#3901fda0 wants to merge 1 commit into
Conversation
|
|
| ; RUN: llvm-spirv %t.spv -o %t.spt --to-text | ||
| ; RUN: FileCheck < %t.spt %s --check-prefix=CHECK-SPIRV | ||
| ; The E2M1INTEL builtins must produce the SPV_INTEL_float4 type (encoding 6214), | ||
| ; not the SPV_EXT_ocp_microscaling_types Float4E2M1EXT type (encoding 4225). |
There was a problem hiding this comment.
Nit: personally I think this comment is redundant, because the produced TypeFloat [[#E2M1Ty:]] 4 6214 says it all.
|
LGTM. I can't say I fully know the context of this PR but I think it makes sense technically. @bashbaug Maybe you know the context better for this one? Please also help review, approve and merge this one if it looks ok to you, as I don't have write permission to approve and merge. |
|
Note, the "INTEL" suffix should be considered part of the instruction name, not part of the type name, so I disagree that this change should be done for some sort of naming reasons. I'd also like to move away from SPV_INTEL_float4 if at all possible in the SPIR-V writer, since we have no plans to productize this extension. Ideally, the SPIR-V writer should be generating SPV_EXT_ocp_microscaling_types in all cases, instead. We can continue to support SPV_INTEL_float4 in the SPIR-V reader for some time if needed for backwards compatibility, but eventually we should move away from it, also. I suggest:
|
fda0
changed the title
|
@bashbaug @Qi-Ye-079 thanks for the feedback. Pushed an update that is based on previous commit 6734efd This should be scoped to SPIRV-Reader. The change now simply allows us to use |
| if (LookupOC == internal::OpStochasticRoundFToFINTEL && | ||
| DstEnc == FPEncodingWrap::E2M1INTEL) | ||
| LookupDstEnc = FPEncodingWrap::E2M1; |
There was a problem hiding this comment.
This is done conditionally right now, only for OpStochasticRoundFToFINTEL, but might we want it to be done unconditionally to fix all usages of the Intel-specific encoding?
We might want to do the same for the source encoding, also.
Even if we decide to do this, I'm fine doing it in a separate PR.
There was a problem hiding this comment.
I don't know to be honest why only this case is an issue - but with this conditional fix for OpStochasticRoundFToFINTEL we have full backwards compatibility for SPV_INTEL_float4 right now (at least that's what testing shows). So broadening this is not strictly needed but I can prepare follow up a PR if you think it's a good idea.
| ; RUN: FileCheck < %t.rev.ll %s --check-prefix=CHECK-LLVM | ||
|
|
||
| ; CHECK-LLVM: call spir_func i4 @_Z46__builtin_spirv_StochasticRoundFP16ToE2M1INTELDhi(half 1.000000e+00, i32 1) | ||
| ; CHECK-LLVM: call spir_func i4 @_Z46__builtin_spirv_StochasticRoundBF16ToE2M1INTELDF16bi(bfloat 1.000000e+00, i32 1) |
There was a problem hiding this comment.
Nit: you don't need --check-prefix=CHECK-LLVM here; a plain CHECK will suffice.
For backward compatibility of the SPIRV Reader only: older modules emitted the `SPV_INTEL_float4` `Float4E2M1INTEL` type (6214) as the `StochasticRoundFToFINTEL` result. Map it to the same builtin as the current `SPV_EXT_ocp_microscaling_types` `Float4E2M1EXT` type (4225). The Writer keeps unchanged and emits the EXT type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
For backward compatibility of the SPIRV Reader only: older modules
emitted the
SPV_INTEL_float4Float4E2M1INTELtype (6214) as theStochasticRoundFToFINTELresult. Map it to the same builtin as thecurrent
SPV_EXT_ocp_microscaling_typesFloat4E2M1EXTtype (4225).The Writer keeps unchanged and emits the EXT type.