[PATCH] D131028: [AArch64] Fix cost model for FADD vector reduction

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2746
+    unsigned BitsPerElement = EltType->getScalarSizeInBits();
+    unsigned ElementsPerRegister = 128 / BitsPerElement;
+
----------------
It would probably be slightly cleaner to use `getRegisterBitWidth` instead of hardcoding the register size.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/reduce-fadd.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
-; RUN: opt -passes='print<cost-model>' 2>&1 -disable-output -mtriple=aarch64--linux-gnu < %s | FileCheck %s
+; RUN: opt -passes='print<cost-model>' 2>&1 -disable-output -mtriple=aarch64-linux-gnu < %s | FileCheck %s
 
----------------
nit: why change this?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/reduce-fadd.ll:12
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 18 for instruction: %fadd_v4f64 = call double @llvm.vector.reduce.fadd.v4f64(double 0.000000e+00, <4 x double> undef)
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 12 for instruction: %fadd_v4f8 = call bfloat @llvm.vector.reduce.fadd.v4bf16(bfloat 0xR0000, <4 x bfloat> undef)
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 20 for instruction: %fadd_v4f128 = call fp128 @llvm.vector.reduce.fadd.v4f128(fp128 undef, <4 x fp128> undef)
----------------
nit: It would be good to have those committed separately, and then have their cost unchanged in this diff.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/reduce-fadd.ll:77
   %fadd_v7f64 = call fast double @llvm.vector.reduce.fadd.v7f64(double 0.0, <7 x double> undef)
   %fadd_v9f64_reassoc = call reassoc double @llvm.vector.reduce.fadd.v9f64(double 0.0, <9 x double> undef)
 
----------------
Does the efficient lowering rely on `-0.0` to be the initial value? With any other start value, we would need at least one additional add, right? Not sure if it is worth to include this in the cost, but we might as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131028