[PATCH] D81472: [Matrix] Update load/store intrinsics.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 4 inline comments as done.
fhahn added a comment.

In D81472#2093178 <https://reviews.llvm.org/D81472#2093178>, @anemet wrote:

> Is there a test for passing alignment?


not yet, will add as a follow up along with the implementation to respect them (should be ready soon).

In D81472#2093172 <https://reviews.llvm.org/D81472#2093172>, @nicolasvasilache wrote:

> @fhahn see https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td#L203
>
> you can extract the LLVM dialect from an LLVM type.
>
> Then you should be able to extract the datalayout with:
>
>   dialect.getLLVMModule().getDataLayout();
>   align = dataLayout.getPrefTypeAlignment(
>       elementTy.cast<LLVM::LLVMType>().getUnderlyingType());
>
>
> see e.g. https://github.com/llvm/llvm-project/blob/master/mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp#L135
>
> you would then need to update this location: https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td#L828
>
> would that work for you ?


@nicolasvasilache  Yeah, I initially was not sure how to create a `false` constant there, but I'll just go ahead and update the MLIR definition to conform to the update in one go, unless you think that will cause problems.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1439-1450
+def int_matrix_transpose
+  : Intrinsic<[llvm_anyvector_ty],
+              [LLVMMatchType<0>, llvm_i32_ty, llvm_i32_ty],
+              [IntrNoMem, IntrSpeculatable, IntrWillReturn, ImmArg<ArgIndex<1>>,
+               ImmArg<ArgIndex<2>>]>;
+
+def int_matrix_multiply
----------------
anemet wrote:
> If this is only reformatting, I would leave that to a separate patch.
done in 1d33c09f220ea9fe2846813bafc46dc5d9394577


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1465
+               WriteOnly<ArgIndex<1>>, ImmArg<ArgIndex<3>>,
+               ImmArg<ArgIndex<4>>, ImmArg<ArgIndex<5>>]>;
 
----------------
jdoerfert wrote:
> anemet wrote:
> > sstefan1 wrote:
> > > jdoerfert wrote:
> > > > fhahn wrote:
> > > > > jdoerfert wrote:
> > > > > > [Drive by][unrelated] I think we should add `nocapture` to the ptr argument and `nosync` to all of them (until we have the white/blacklist for intrinsics with sensible defaults).
> > > > > Thanks for pointing that out. There are just too many attributes to keep track of. I wish we had some kind of attribute 'group' to say: just reads/write from the pointer, no capture and other stuff
> > > > We (@sstefan1) proposed a whitelist and blacklist approach for intrinsics before. Hasn't gone anywhere yet. For the OpenMP runtime functions we actually have such attribute groups. Either way is better than what we do so far.
> > > I will try to get back to that soon.
> > Has this been addressed?
> This is unrelated and should not block the patch. Eventually we want easier attribute handling for intrinsics but we are still working on it.
I added to nosync/nocapture attributes in 1d33c09f220ea9fe2846813bafc46dc5d9394577


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81472