[Mlir-commits] [mlir] [mlir][vector][memref] Add `alignment` attribute to memory access ops (PR #144344)

[Andrzej Warzyński]

banach-space wrote:

> Currently, the lowerings of vector.load and vector.store to LLVM always assume that the pointer is aligned to the natural alignment of the vector element, not the vector. This is behavior users often want to override, either in general or on a case-by-case basis, and having an alignment attribute lets people do that

Recently (see https://github.com/llvm/llvm-project/pull/137389), `ConvertVectorToLLVM` was extended to support exactly that kind of override via the `--convert-vector-to-llvm='use-vector-alignment=1' `flag. Isn’t this patch duplicating that logic?

Let’s ask a specific question: could `--convert-vector-to-llvm='use-vector-alignment=1'` be replaced by what’s being added here? If so, great - let’s unify. If not, let’s clarify how they interact.

(Ping @sstamenova for context.)

> Again, my concern here is we keep adding alternative ways to do things thus increasing overall complexity of the system. These new ways may be more flexible or general than existing mechanisms, but we must clean those up and ensure some overall coherence.

+1

> > I think this'll be a good moment to clarify what alignment means for sub-byte types, since we have EmulateNarrowTypes. I'd argue that if the type in the memref has width < 8 bits, then alignment should be in units of the number of elements, not the number of bytes. That'll allow the narrow type emulator to use faster lowerings when you, for example, do a vector.store %v, %m[[...], {alignment = 2 : index} : vector<2xi4>, memref<...xi4>
> 
> Please bring this to discourse.

+1

IMHO, narrow-type emulation is a poor example here. While many patterns in that pass deal with sub-byte types, some also emulate byte-sized elements (see discussion in https://github.com/llvm/llvm-project/issues/131529). That logic has grown organically and lacks cohesion - it’s not an ideal foundation for defining alignment semantics.

---

To be clear, I’m not looking to block this patch - but we now have multiple mechanisms to control alignment:

* The new attributes on `vector.load` / `vector.store` (this patch)
* The `--convert-vector-to-llvm='use-vector-alignment=1'` flag
* `memref.assume_alignment`, which introduces its own complications (see [#144809](https://github.com/llvm/llvm-project/pull/144809), [#144825](https://github.com/llvm/llvm-project/issues/144825))

At the very least, this patch should clarify the relationship between the new attribute and the vector alignment flag in the lowering pass. Otherwise, we risk further fragmentation of behaviour and assumptions around alignment.

https://github.com/llvm/llvm-project/pull/144344