[Mlir-commits] [mlir] [MLIR] Implement emulation of static indexing subbyte type vector stores (PR #115922)

[llvmlistbot at llvm.org]

lialan wrote:

@banach-space 

> shouldn't `vector.load` also be atomic?

Should we allow `vector.load` to be atomic? I do think in some cases we don't but in some other extreme cases we might need it to be bug-free. If the target supports loads that are bigger than the vector size (for example, on a target that supports unaligned loading of 64bit, while the loaded vector size is only 32bit), then we are free to not use atomic ops. If we know that the vector load is part of affine accesses which guarantees data parallelism then we also don't need to worry about atomic. However, such two cases do not cover all possible cases.

So I think if we don't know, just give the control to the upper layer. We could implement a bool/option just like `vector.store` does and let the caller to decide. But for now, we haven't encountered a case where this is necessary. So... maybe a future improvement?

> Also, IIUC, the main difference between atomic and non-atomic stores is the presence of memref.atomic_rmw? Perhaps the tests for "atomic" stores could be simplified to only check that the correct ops are generated?

The two are very similar but are different, and are emitted by two different functions. So it is best to test them separately I think. We could probably refactor and merge some common parts later, I do have some plan to gradually make the code better.

> Last, but not least, could you make sure that tests for vector.store and vector.load are symmetric? The test coverage for these ops should be identical (within reasons). That can only be verified when tests are ... symmetric :)

Done.

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