vector.transfer_read and vector.transfer_write operations are converted
to llvm intrinsics with specific alignment information, however there
doesn't seem to be a way in llvm to take information from llvm.assume
intrinsics and change this alignment information. In any
event, due the to the structure of the llvm.assume instrinsic, applying
this information at the llvm level is more cumbersome. Instead, let's
generate the masked vector load and store instrinsic with the right
alignment information from MLIR in the first place. Since
we're bothering to do this, lets just emit the proper alignment for
loads, stores, scatter, and gather ops too.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
---|---|---|
123 | Can we fold this into a: LogicalResult getMemRefAlignment(LLVMTypeConverter &typeConverter, Value memRef, unsigned &align) { MemRefType memrefType = memRef.getType().cast<MemRefType>(); // old code // your code } ? | |
130 | align = std::max(align, op.alignment()) (with a cast if needed) ? |
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
---|---|---|
123 | +1 with probably a name to reflect that getMemRefAlignmentAndUpdate() or something like that |
In incorporating your feedback, I decided to emit the proper alignment other loads and stores too. Thoughts?
Looks good!
Now I'm wondering whether we should go for the LICM instead?
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
---|---|---|
123 | If no assumed alignment -> Return the minimal alignment value that satisfies all the AssumeAlignment uses of `value`. If no such uses exist, return 0. | |
130 | Thanks for re-spelling. |
And by licm I mean lcm :p ..
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
---|---|---|
130 | And by licm I mean lcm :p .. |
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
---|---|---|
130 | Probably.. I guess I was implicitly assuming everything was a power of 2. |
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp | ||
---|---|---|
130 | Sure enough : 'error: 'memref.assume_alignment' op alignment must be power of 2' |
mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir | ||
---|---|---|
1555 | How can we know that this store is aligned on 32B without knowing anything about %i and %j. |
mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir | ||
---|---|---|
1555 | This one is clearly not aligned on 32B but it should be aligned on: lcm(annotation + offset_alignment, most_minor_dim_alignment) == lcm(32 + 0 * 4, 100 * 4) == 16B There seems indeed to be a small bug somewhere but I would prefer we fix rather than flatly revert. |
mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir | ||
---|---|---|
1555 | Why? Revert is conservative and cheap: it is trivial to reapply the patch with the fix without pressure. |
Fair enough, my bias was against losing the contribution; didn't think about it your way, thanks!
mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir | ||
---|---|---|
1555 | Yes Stephen reverted it as a quick solution as this was causing miscompile on IREE side. |
mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir | ||
---|---|---|
1555 | You're right, I was very focused on the memref type itself and did not look deeper at the indexing.. What I meant to say is that the alignment for each start of every row of memref is already degraded wrt the base pointer. More precisely: @%memref[0, 0] is aligned at 32B @%memref[1, 0] is aligned at 16B but not 32B @%memref[2, 0] is aligned at 32B ... the indexing within the row can indeed be any 4B aligned. But even worse, even @%memref[1, 32] is aligned at 16B and not 32B. This is one of the important things to grasp when talking about memref and alignment: a memref is not a pointer. (n-1)-D array of pointers to 1-D row (even if they are not materialized) Please ignore my earlier comment on the revert and sorry about the noise 😛 |
Can we fold this into a:
?