[PATCH] D140392: [SLP][AArch64] Incorrectly estimated intrinsic as a function call

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7297-7318
+      bool NoCallIntrinsic = false;
+      if (isa<IntrinsicInst>(&*PrevInstIt)) {
+        auto *II = dyn_cast<IntrinsicInst>(&*PrevInstIt);
+        CallInst *CI = cast<CallInst>(II);
+        SmallVector<Type *, 4> Tys;
+        for (auto &ArgOp : CI->args())
+          Tys.push_back(ArgOp->getType());
----------------
Can you tranform all this code to lambda?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7298-7299
+      bool NoCallIntrinsic = false;
+      if (isa<IntrinsicInst>(&*PrevInstIt)) {
+        auto *II = dyn_cast<IntrinsicInst>(&*PrevInstIt);
+        CallInst *CI = cast<CallInst>(II);
----------------
```
if (auto *II = dyn_cast<IntrinsicInst>(&*PrevInstIt))
```


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7301-7303
+        SmallVector<Type *, 4> Tys;
+        for (auto &ArgOp : CI->args())
+          Tys.push_back(ArgOp->getType());
----------------
Sink this code to the `else` substatement.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7305-7307
+        if (II->isAssumeLikeIntrinsic())
+          NoCallIntrinsic = true;
+        else {
----------------
Braces


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7313-7318
+          InstructionCost IntrCost =
+              TTI->getIntrinsicInstrCost(ICA, TTI::TCK_RecipThroughput);
+          InstructionCost CallCost = TTI->getCallInstrCost(
+              nullptr, II->getType(), Tys, TTI::TCK_RecipThroughput);
+          if (IntrCost < CallCost)
+            NoCallIntrinsic = true;
----------------
Do we have a btter way to check if the intrinsic is lowered as an instruction rather than the call?
```
NoCallIntrinsic = IntrCost < CallCost;
```


================
Comment at: llvm/test/Transforms/SLPVectorizer/AArch64/fmulladd.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=slp-vectorizer -mtriple=aarch64-unknown-unknown -mcpu=cortex-a53 -S | FileCheck %s
----------------
Need to precommit the test


================
Comment at: llvm/test/Transforms/SLPVectorizer/AArch64/fmulladd.ll:3-4
+; RUN: opt < %s -passes=slp-vectorizer -mtriple=aarch64-unknown-unknown -mcpu=cortex-a53 -S | FileCheck %s
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-unknown"
+
----------------
I assume this can be removed


================
Comment at: llvm/test/Transforms/SLPVectorizer/AArch64/fmulladd.ll:9
+
+define void @foo(ptr nocapture noundef readonly %d, ptr nocapture noundef readonly %e) {
+; CHECK-LABEL: @foo(
----------------
cleanup attrs, I assume they are not required.


================
Comment at: llvm/test/Transforms/SLPVectorizer/AArch64/fmulladd.ll:75
+
+declare double @llvm.fmuladd.f64(double, double, double) #1
----------------
Remove attribute reference


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

https://reviews.llvm.org/D140392