Bug 49739 - [Matrix] Support #pragma clang fp

[effective-light]

Summary

From https://bugs.llvm.org/show_bug.cgi?id=49739:

Currently, #pragma clang fp are ignored for matrix types.

For the code below, the contract fast-math flag should be added to the generated call to llvm.matrix.multiply and fadd

typedef float fx2x2_t __attribute__((matrix_type(2, 2)));

void foo(fx2x2_t &A, fx2x2_t &C, fx2x2_t &B) {
  #pragma clang fp contract(fast)
  C = A*B + C;
}

[fhahn]

Awesome, thanks for the patch! I'm also adding @mibintc who I think was working on adding support for the pragma & co.

Could you add a brief description to the patch and also reference the bug report on bugs.llvm.org?

clang/test/CodeGen/fp-matrix-pragma.c

2

I don't think we need --target=arm64-unknown-iphoneos or -g0?

24

could you also add a float test that has both contract(fast) reassociate(on)?

[effective-light]

Remove the unnecessary flags and add another test

[mibintc]

The diff appears to be 2 separate commits, so on first glance this is only patching the test files. Usually if I am working on a patch and have responded to comments, I compress the patch+updates into a single commit (git rebase -i) before creating a diff to upload to Phabricator.

Are there Matrix builtins that you wish to have decorated with the FMF? e.g. look at CGBuiltin.cpp

[effective-light]

Whoops

[kpn]

I don't know the matrix implementation so I can't swear this hits every place needed, but the uses of CGFPOptionsRAII in this patch look correct at least.

[effective-light]

Fix build

[effective-light]

Use the correct clang command.

[fhahn]

In D100834#2702550, @kpn wrote:

I don't know the matrix implementation so I can't swear this hits every place needed, but the uses of CGFPOptionsRAII in this patch look correct at least.

Other parts of the extension include __builtin_matrix_transpose, indexing into a matrix and casting, but I don't think the FMFs are needed there. One thing that would be good to also test would be the compound operators, (-=, +=, *=). @effective-light it would be great if you could add a test for those, then LGTM from my side. If you need someone to commit the change on your behalf, please let us know and share the name + email to use for the commit authorship.

[effective-light]

Add a test for compound operations

[effective-light]

In D100834#2704330, @fhahn wrote:

In D100834#2702550, @kpn wrote:

I don't know the matrix implementation so I can't swear this hits every place needed, but the uses of CGFPOptionsRAII in this patch look correct at least.

Other parts of the extension include __builtin_matrix_transpose, indexing into a matrix and casting, but I don't think the FMFs are needed there. One thing that would be good to also test would be the compound operators, (-=, +=, *=). @effective-light it would be great if you could add a test for those, then LGTM from my side. If you need someone to commit the change on your behalf, please let us know and share the name + email to use for the commit authorship.

Ya, you can commit it under name: Hamza Mahfooz, email: [e-mail], thanks.

[fhahn]

LGTM, thanks!

I'll commit the patch shortly.

[fhahn]

@effective-light just FYI, if you are interested in improving fast-math flags handling during matrix lowering on the LLVM side, there's https://bugs.llvm.org/show_bug.cgi?id=49738

clang/lib/CodeGen/CGExprScalar.cpp

if (Ops.Ty->isConstantMatrixType()) {
       llvm::MatrixBuilder<CGBuilderTy> MB(Builder);
       // We need to check the types of the operands of the operator to get the
       // correct matrix dimensions.
       auto *BO = cast<BinaryOperator>(Ops.E);
       auto *LHSMatTy = dyn_cast<ConstantMatrixType>(
           BO->getLHS()->getType().getCanonicalType());
       auto *RHSMatTy = dyn_cast<ConstantMatrixType>(
           BO->getRHS()->getType().getCanonicalType());
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, Ops.FPFeatures);
       if (LHSMatTy && RHSMatTy)
         return MB.CreateMatrixMultiply(Ops.LHS, Ops.RHS, LHSMatTy->getNumRows(),
                                        LHSMatTy->getNumColumns(),
                                        RHSMatTy->getNumColumns());
       return MB.CreateScalarMultiply(Ops.LHS, Ops.RHS);
     }
 
     if (Ops.Ty->isUnsignedIntegerType() &&
if (Ops.Ty->isConstantMatrixType()) {
     // correct matrix dimensions.
     auto *BO = cast<BinaryOperator>(Ops.E);
     (void)BO;
     assert(
         isa<ConstantMatrixType>(BO->getLHS()->getType().getCanonicalType()) &&
         "first operand must be a matrix");
     assert(BO->getRHS()->getType().getCanonicalType()->isArithmeticType() &&
            "second operand must be an arithmetic type");
+    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, Ops.FPFeatures);
     return MB.CreateScalarDiv(Ops.LHS, Ops.RHS,
                               Ops.Ty->hasUnsignedIntegerRepresentation());
   }
 
   if (Ops.LHS->getType()->isFPOrFPVectorTy()) {
     llvm::Value *Val;
     CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, Ops.FPFeatures);
     Val = Builder.CreateFDiv(Ops.LHS, Ops.RHS, "div");
case LangOptions::SOB_Trapping:
       if (CanElideOverflowCheck(CGF.getContext(), op))
         return Builder.CreateNSWAdd(op.LHS, op.RHS, "add");
       return EmitOverflowCheckedBinOp(op);
     }
   }
 
   if (op.Ty->isConstantMatrixType()) {
     llvm::MatrixBuilder<CGBuilderTy> MB(Builder);
+    CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, op.FPFeatures);
     return MB.CreateAdd(op.LHS, op.RHS);
   }
 
   if (op.Ty->isUnsignedIntegerType() &&
       CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
       !CanElideOverflowCheck(CGF.getContext(), op))
     return EmitOverflowCheckedBinOp(op);
 
if (op.Ty->isSignedIntegerOrEnumerationType()) {
         if (CanElideOverflowCheck(CGF.getContext(), op))
           return Builder.CreateNSWSub(op.LHS, op.RHS, "sub");
         return EmitOverflowCheckedBinOp(op);
       }
     }
 
     if (op.Ty->isConstantMatrixType()) {
       llvm::MatrixBuilder<CGBuilderTy> MB(Builder);
+      CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, op.FPFeatures);
       return MB.CreateSub(op.LHS, op.RHS);
     }
 
     if (op.Ty->isUnsignedIntegerType() &&
         CGF.SanOpts.has(SanitizerKind::UnsignedIntegerOverflow) &&
         !CanElideOverflowCheck(CGF.getContext(), op))
       return EmitOverflowCheckedBinOp(op);
 

clang/test/CodeGen/fp-matrix-pragma.c

+// RUN: %clang -emit-llvm -S -fenable-matrix -mllvm -disable-llvm-optzns %s -o - | FileCheck %s
+
+typedef float fx2x2_t __attribute__((matrix_type(2, 2)));
+typedef int ix2x2_t __attribute__((matrix_type(2, 2)));
+
+fx2x2_t fp_matrix_contract(fx2x2_t a, fx2x2_t b, float c, float d) {
+// CHECK: call contract <4 x float> @llvm.matrix.multiply.v4f32.v4f32.v4f32
+// CHECK: fdiv contract <4 x float>
+// CHECK: fmul contract <4 x float>
+#pragma clang fp contract(fast)
+  return (a * b / c) * d;
+}
+
+fx2x2_t fp_matrix_reassoc(fx2x2_t a, fx2x2_t b, fx2x2_t c) {
+// CHECK: fadd reassoc <4 x float>
+// CHECK: fsub reassoc <4 x float>
+#pragma clang fp reassociate(on)
+  return a + b - c;
+}
+
+fx2x2_t fp_matrix_ops(fx2x2_t a, fx2x2_t b, fx2x2_t c) {
+// CHECK: call reassoc contract <4 x float> @llvm.matrix.multiply.v4f32.v4f32.v4f32
+// CHECK: fadd reassoc contract <4 x float>
+#pragma clang fp contract(fast) reassociate(on)
+  return a * b + c;
+}
+
+ix2x2_t int_matrix_ops(ix2x2_t a, ix2x2_t b, ix2x2_t c) {
+// CHECK: call <4 x i32> @llvm.matrix.multiply.v4i32.v4i32.v4i32
+// CHECK: add <4 x i32>
+#pragma clang fp contract(fast) reassociate(on)
+  return a * b + c;
+}