[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[]{
----------------
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. :)


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1829
+  case ISD::OR:
+    if (const auto *Entry = CostTableLookup(CostTblNoPairwise, ISD, MTy)) {
+      auto *ValVTy = cast<FixedVectorType>(ValTy);
----------------
Nit: to reduce indentation and increase readability, can you return early here:

  const auto *Entry = CostTableLookup(CostTblNoPairwise, ISD, MTy);
  if (!Entry)
    break;
  ...


================
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) &&
----------------
If this cast can fail, we are accessing a null pointer below?


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

https://reviews.llvm.org/D104538