Page MenuHomePhabricator

[Matrix] Make matrix.multiply variadic, accept optional NUW/NSW flags.
AbandonedPublic

Authored by fhahn on May 18 2020, 7:55 AM.

Details

Summary

This patch extends the llvm.matrix.multiply intrinsic to accept variadic
arguments. This gives us greater flexibility in terms of optional
arguments. As a first use case, 2 optional bools can provided additional
arguments, to indicate that the NUW/NSW flags should be added to the
generated integer instructions.

The verifier is extended to limit the variadic arguments to exactly what
is specified in the LangRef.

Diff Detail

Unit TestsFailed

TimeTest
130 msMLIR.Target::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/bin/mlir-translate -mlir-to-llvmir /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/mlir/test/Target/llvmir-intrinsics.mlir | /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/BETA_amd64_debian_testing_clang8/llvm-project/mlir/test/Target/llvmir-intrinsics.mlir

Event Timeline

fhahn created this revision.May 18 2020, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2020, 7:55 AM
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
958

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.

LuoYuanke added inline comments.May 18 2020, 5:55 PM
llvm/docs/LangRef.rst
15198

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.

fhahn marked 3 inline comments as done.May 19 2020, 4:38 AM
fhahn added inline comments.
llvm/docs/LangRef.rst
15198

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.

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
958

yeah, I've added a MultiplyOptions struct. Should help in the future, also with other fast-math flags/constrained math options.

fhahn updated this revision to Diff 264852.May 19 2020, 4:39 AM
fhahn marked an inline comment as done.

Add MultiplyOptions struct.

No worries. I just wanted to point out that there are other efforts to define floating-point behaviour beyond fast-math.

fhahn added a comment.Jun 4 2020, 8:08 AM

I've put up an alternative approach, which uses operand bundles instead of variadic arguments: D81166 The approach in the new patch seems more lightweight, explicit and flexible.

fhahn abandoned this revision.Jul 15 2020, 9:35 AM

It seems like the operand bundle approach is preferred. I'll abandon this patch for now.