[AArch64][SVE] Replace fmul, fadd and fsub LLVM IR instrinsics with LLVM IR binary ops

[MattDevereau]

Summary

Replacing instrinsics with normal binops results in
more succinct AArch64 SVE output, e.g.:

4:   65428041   fmul    z1.h, p0/m, z1.h, z2.h
8:   65408020   fadd    z0.h, p0/m, z0.h, z1.h
->
4:   65620020   fmla    z0.h, p0/m, z1.h, z2.h

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

780

This is not enough as you're not considering the predicate the intrinsic takes. We can only do this transform when the predicate is the equivalent of ptrue all.

869–870

It seems odd for aarch64_sve_fadd to call instCombineSVEVectorMul. That said the functionality you're after is pretty generic so perhaps it's worth creating instCombineSVEVectorBinOp as I can see us extending this for other cases. This can be called directly for case Intrinsic::aarch64_sve_fadd and called at the bottom of instCombineSVEVectorMul whilst we figure out how much of that function is still useful.

[MattDevereau]

Created new method instCombineSVEVectorBinOp and narrowed the intrinsic conditional replacement to svp_true_xx/Intrinsic::aarch64_sve_convert_from_svbool

[MattDevereau]

Added ptrue check

[bsmith]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

702–707

This is not taking into consideration the type of the ptrue, this could be an i64 type ptrue passed to an i32 type mul/add, hence does not cover all lanes of the operation. There needs to be a check in here that checks that the ptrue type is the same or smaller than the predicated operation. For example, for a <vscale x 4 x i32> vector op, only ptrue types of <vscale x 4 x i1>, <vscale x 8 x i1>, <vscale x 16 x i1> are allowed, <vscale x 2 x i1> is not.

[peterwaller-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

703

I would prefer to see if (match(II, m_Intrinsic(Intrinsic::aarch64_sve_fmul))) && ... which is more idiomatic rather than introducing an IsIntrinsic lambda.

[MattDevereau]

Added test, removed Intrinsic::aarch64_sve_from_svbool case

[MattDevereau]

Make instCombineSVEVectorBinOp more succinct

[MattDevereau]

updated for clang-format and clang-tidy

[david-arm]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

700

You could restructure this as:

auto BinOpCode = intrinsicIDToBinOpCode(II.getIntrinsicID());
if (BinOpCode  == Instruction::BinaryOpsEnd ||
    !match(...))
  return None;

IRBuilder<> Builder(II.getContext());
...
return IC.replaceInstUsesWith

If you agree that looks better?

870

Since we're doing this for fadd shall we also do this for fsub too? It's just literally adding another case statement I think.

[MattDevereau]

Added fsub case and tests. Rearranged instCombineSVEVectorBinOp logic

[david-arm]

Hi @MattDevereau, thanks for making the changes - it looks really good now! Is it worth adding at least one negative test for the case where the predicate isn't ptrue all?

[MattDevereau]

added test no_replace_on_non_ptrue to assert only ptrue_all is replaced

[MattDevereau]

renamed no_replace_on_non_ptrue to no_replace_on_non_ptrue_all

[david-arm]

LGTM! Thanks for making the changes.

llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-fma-binops.ll

10 ↗

(On Diff #376276)

nit: Could you change the test names before committing to have something different to the intrinsic name? i.e. you could replace the '.' with '_' so that you have

declare <vscale x 8 x half> @llvm_aarch64_sve_fmul_nxv8f16(

[MattDevereau]

Closed in commit f085a9db8b8d408d08adcba8e283e637a0116622

@david-arm changing the function declarations to underscores broke the tests, so i've left them for now

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

if (Op1 && Op2 &&
 
     PTest->takeName(&II);
     return IC.replaceInstUsesWith(II, PTest);
   }
 
   return None;
 }
 
+static Optional<Instruction *> instCombineSVEVectorBinOp(InstCombiner &IC,
+                                                         IntrinsicInst &II) {
+  auto IsIntrinsic = [](auto *I, auto In) {
+    auto *IntrI = dyn_cast<IntrinsicInst>(I);
+    if (!IntrI || IntrI->getIntrinsicID() != In)
+      return false;
+    return true;
+  };
+
+  auto *OperandValue = II.getOperand(0);
+  if (IsIntrinsic(OperandValue, Intrinsic::aarch64_sve_convert_from_svbool)) {
+    OperandValue = dyn_cast<IntrinsicInst>(OperandValue)->getOperand(0);
+  }
+
+  if (IsIntrinsic(OperandValue, Intrinsic::aarch64_sve_ptrue)) {
+    auto *PTrueIntrinsic = dyn_cast<IntrinsicInst>(OperandValue);
+    const uint64_t PTruePattern =
+        cast<ConstantInt>(PTrueIntrinsic->getOperand(0))->getZExtValue();
+    if (PTruePattern != AArch64SVEPredPattern::all) {
+      return None;
+    }
+
+    Instruction::BinaryOps BinOp = Instruction::BinaryOpsEnd;
+    switch (II.getIntrinsicID()) {
+    case Intrinsic::aarch64_sve_fmul:
+      BinOp = Instruction::BinaryOps::FMul;
+      break;
+    case Intrinsic::aarch64_sve_fadd:
+      BinOp = Instruction::BinaryOps::FAdd;
+      break;
+    default:
+      return None;
+    }
+
+    IRBuilder<> Builder(II.getContext());
+    Builder.SetInsertPoint(&II);
+    return IC.replaceInstUsesWith(
+        II, Builder.CreateBinOp(BinOp, II.getOperand(1), II.getOperand(2)));
+  }
+
+  return None;
+}
+
 static Optional<Instruction *> instCombineSVEVectorMul(InstCombiner &IC,
                                                        IntrinsicInst &II) {
   auto *OpPredicate = II.getOperand(0);
   auto *OpMultiplicand = II.getOperand(1);
   auto *OpMultiplier = II.getOperand(2);
 
   IRBuilder<> Builder(II.getContext());
   Builder.SetInsertPoint(&II);
if (IsUnitDupX(OpMultiplier)) {
     auto *DupInst = cast<IntrinsicInst>(OpMultiplier);
     auto *DupPg = DupInst->getOperand(1);
     // TODO: this is naive. The optimization is still valid if DupPg
     // 'encompasses' OpPredicate, not only if they're the same predicate.
     if (OpPredicate == DupPg) {
       OpMultiplicand->takeName(&II);
       return IC.replaceInstUsesWith(II, OpMultiplicand);
     }
   }
 
-  return None;
+  return instCombineSVEVectorBinOp(IC, II);
 }
 
 static Optional<Instruction *> instCombineSVEUnpack(InstCombiner &IC,
                                                     IntrinsicInst &II) {
   IRBuilder<> Builder(II.getContext());
   Builder.SetInsertPoint(&II);
   Value *UnpackArg = II.getArgOperand(0);
   auto *RetTy = cast<ScalableVectorType>(II.getType());
AArch64TTIImpl::instCombineIntrinsic(InstCombiner &IC,
   case Intrinsic::aarch64_sve_cntb:
     return instCombineSVECntElts(IC, II, 16);
   case Intrinsic::aarch64_sve_ptest_any:
   case Intrinsic::aarch64_sve_ptest_first:
   case Intrinsic::aarch64_sve_ptest_last:
     return instCombineSVEPTest(IC, II);
   case Intrinsic::aarch64_sve_mul:
   case Intrinsic::aarch64_sve_fmul:
     return instCombineSVEVectorMul(IC, II);
+  case Intrinsic::aarch64_sve_fadd:
+    return instCombineSVEVectorBinOp(IC, II);
   case Intrinsic::aarch64_sve_tbl:
     return instCombineSVETBL(IC, II);
   case Intrinsic::aarch64_sve_uunpkhi:
   case Intrinsic::aarch64_sve_uunpklo:
   case Intrinsic::aarch64_sve_sunpkhi:
   case Intrinsic::aarch64_sve_sunpklo:
     return instCombineSVEUnpack(IC, II);
   }