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

[Vir Narula via Phabricator via llvm-commits]

virnarula marked 4 inline comments as done.
virnarula added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2591
       {ISD::ADD, MVT::v2i64,  2},
-      {ISD::FADD,MVT::v4f16,  2},
-      {ISD::FADD,MVT::v8f16,  2},
----------------
fhahn wrote:
> Those don't appear in the current version on `main`. Is it possible that the patch has been applied on top of some other local changes?
Yes that's what it seems like.


================
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
 
----------------
fhahn wrote:
> nit: why change this?
undoing


================
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)
 
----------------
fhahn wrote:
> 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.
So in the getReductionCost function, we don't seem to have access to any of the arguments which makes this difficult. In the test cases, I was thinking we might just want to make all the 0.0 and -0.0 into undefs to avoid confusion for anyone reading the test cases. what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131028