[PATCH] D104538: [CostModel][AArch64] Improve cost model for vector reduction intrinsics

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1793
   // instructions as normal vector adds. This is the only arithmetic vector
   // reduction operation for which we have an instruction.
   static const CostTblEntry CostTblNoPairwise[]{
----------------
RosieSumpter wrote:
> SjoerdMeijer wrote:
> > You can say something here how this looks like for these new opcodes that we are adding.
> > 
> > I would also appreciate a pointer to the codegen test so that's easy to verify this. I was going to say that a cost of 15 looks high, but I was too lazy to find the codegen test. :)
> There is a codegen test for i1 types (test/CodeGen/AArch64/vecreduce-bool.ll) but I couldn't find a test for the other vector types. These values are just from a manual check I did of the codegen (e.g. llc reduce-or.ll -mtriple=aarch64) 
In that case, would you mind adding those codegen tests (separately)?
You can then refer to that here in a comment.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1830
+    if (const auto *Entry = CostTableLookup(CostTblNoPairwise, ISD, MTy)) {
+      auto *ValVTy = cast<FixedVectorType>(ValTy);
+      if (!ValVTy->getElementType()->isIntegerTy(1) &&
----------------
RosieSumpter wrote:
> SjoerdMeijer wrote:
> > If this cast can fail, we are accessing a null pointer below?
> I don't think a failed cast would make it this far since the cast calls assert() doesn't it? If it still needs checking, could do an if(ValVTy) before the following code?
Ah yes, I read this as a dyn_cast, but it's a cast, so is correct.


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

https://reviews.llvm.org/D104538