[PATCH] D80124: [Matrix] Make matrix.multiply variadic, accept optional NUW/NSW flags.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/docs/LangRef.rst:15198
       declare vectorty @llvm.matrix.multiply.*(vectorty %A, vectorty %B, i32 <M>, i32 <N>, i32 <K>)
+      declare vectorty @llvm.matrix.multiply.*(vectorty %A, vectorty %B, i32 <M>, i32 <N>, i32 <K>, i1 <HasNUW>, i1 <HasNSW>)
 
----------------
LuoYuanke wrote:
> It reminds me that for float type we may have more flags to support. Do we need to have a strict FP version and fast FP version for it? Here is link of fast-math flags. https://llvm.org/docs/LangRef.html#fast-math-flags.
> It reminds me that for float type we may have more flags to support. Do we need to have a strict FP version and fast FP version for it? Here is link of fast-math flags. https://llvm.org/docs/LangRef.html#fast-math-flags.

FMF can be specified directly at the call site and it is already used for selecting fmuladd (https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/LowerMatrixIntrinsics/multiply-double-contraction-fmf.ll#L65). Other flags FMF should probably be passed through directly and there should be no need to use extra arguments for that.

For the constrained FP math mentioned by @tschuett we would need extra arguments to indicate the constrained versions should be used. This should be relatively straight-forward to do, but I probably won't have time to work on this in the next few months at least.


================
Comment at: llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp:936
+                      IRBuilder<> &Builder, bool AllowContraction, bool HasNUW,
+                      bool HasNSW, unsigned &NumComputeOps) {
     NumComputeOps += getNumOps(A->getType());
----------------
nicolasvasilache wrote:
> has this reached a point where you want a separate `LowerMatrixIntrinisicsOptions().setHasNUW(true).setHasNSW(false)`and make this into 
> `createMulAdd(Value *Sum, Value *A, Value *B, LowerMatrixIntrinisicsOptions options)` ?
> 
> As a future user of the potential per-operand row/col major arguments it would make things much nicer IMO.  
yeah, I've added a `MultiplyOptions` struct. Should help in the future, also with other fast-math flags/constrained math options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80124