[PATCH] D155306: [mlir][ArmSME] Add tile load op and extend tile store tile size support

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: mlir/include/mlir/Dialect/ArmSME/IR/ArmSME.td:340
       Arguments<(ins Arg<LDSTPredicate, "Vector predicate">,
-                 Arg<LLVM_AnyPointer, "Load address", [MemRead]>,
+                 Arg<LLVM_AnyPointer, "Load address", [MemRead, MemWrite]>,
                  Arg<I32, "Virtual tile ID">,
----------------
c-rhodes wrote:
> dcaballe wrote:
> > It may be worth asking in Discourse about this. There might be a better side effect or workaround for this.
> > It may be worth asking in Discourse about this. There might be a better side effect or workaround for this.
> 
> Good suggestion will do
> > It may be worth asking in Discourse about this. There might be a better side effect or workaround for this.
> 
> Good suggestion will do

In the process of asking on Discourse I did some digging and found the SME load intrinsics don't get DCE'd in the backend because they have no side-effects and so the [[ 
https://github.com/llvm/llvm-project/blob/17a58b3ca7ec18585e9ea8ed8b39d72fe36fb6cb/llvm/include/llvm/IR/Intrinsics.td#L24 | default worse-cast is assumed ]]

> Intr*Mem - Memory properties.  If no property is set, the worst case
> is assumed (it may read and write any memory it can get access to and it may
> have other side effects).

I've removed the side-effects from the load intrinsics to match the semantics of the backend and they no longer get DCE'd. Thanks for the suggestion to look into this!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155306/new/

https://reviews.llvm.org/D155306