[SYCL][InstCombine] Skip GEP canonicalization for JointMatrix types#21676
[SYCL][InstCombine] Skip GEP canonicalization for JointMatrix types#21676zhaomaosu wants to merge 2 commits intointel:syclfrom
Conversation
GEP canonicalization in visitGetElementPtrInst rewrites single-index GEPs to use an [N x i8] stride based on DL.getTypeAllocSize(). For SPIR-V cooperative matrix types (spirv.CooperativeMatrixKHR), this allocation size is not meaningful — the type is opaque to the data layout — so the canonicalized stride is incorrect and produces invalid IR. Add an IsMatrixType predicate that recognizes GEP element types that are, and skip the i8-stride canonicalization when this predicate matches.
This reverts commit 293c99f.
| // Skip canonicalization for JointMatrix type. DL.getTypeAllocSize() will not | ||
| // return the true allocation size, so the canonicalized [N x i8] stride would | ||
| // be incorrect. | ||
| auto IsMatrixType = [](Type *Ty) -> bool { |
There was a problem hiding this comment.
Should we skip canonicalization for any TargetExtType? Would getTypeAllocSize return true allocation size for other target extension types?
I think we might need to submit this fix to upstream LLVM to avoid unnecessary customizations in intel/llvm.
There was a problem hiding this comment.
it's ok for me to upstream the fix to community, I open this PR mainly for testing purpose.
There was a problem hiding this comment.
it's ok for me to upstream the fix to community, I open this PR mainly for testing purpose.
In this case you should open draft PR.
YuriPlyakhin
left a comment
There was a problem hiding this comment.
it seems fix breaks IGC compiler:
Failed Tests (3):
SYCL :: Matrix/SG32/joint_matrix_prefetch.cpp
SYCL :: Matrix/joint_matrix_bf16_fill_k_cache_prefetch.cpp
SYCL :: Matrix/joint_matrix_prefetch.cpp
There was a problem hiding this comment.
another concern is there are other places, where GEP is canonialized, for example:
https://github.com/zhaomaosu/llvm/blob/6fb20d2f5bcc1610959b4747a2ecd1ef58ef7293/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp#L3404
If we prevent canonicalization for TargetExtType in one place, I think we need to prevent it in all places.
Previously we decided to handle this in backend compilers instead.
If we want to review this decision, it might be good idea to coordinate this change with IGC engineers as well and maybe to submit change like that to upstream LLVM.
|
For failed prefetch tests, I dumped the IR before translated to spirv, looks like the problem mainly lies in these instructions: And I also dumped the IR from good compiler without community change 6ecbc0c96, these instructions should like: The bfloat16 type is also canonicalized to byte array type [2 x i8], but unlike joint matrix type, bfloat16 has determined size, so the IR should be legal. I'm not familiar with joint matrix spec and how IGC compiler handle this case. @dkhaldi, may I get your comments/suggestions. BTW, IGC compiler reported error messages like: |
Since you disable GEP canonicalization, why we still see the array of type [2 x i8]? |
GEP canonicalization in visitGetElementPtrInst rewrites single-index GEPs to use an [N x i8] stride based on DL.getTypeAllocSize(). For SPIR-V cooperative matrix types (spirv.CooperativeMatrixKHR), this allocation size is not meaningful — the type is opaque to the data layout — so the canonicalized stride is incorrect and produces invalid IR.
Add an IsMatrixType predicate that recognizes GEP element types that are, and skip the i8-stride canonicalization when this predicate matches.