[PATCH] D93030: [AArch64][SVE]Add cost model for masked gather and scatter for scalable vector.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

LGTM with nits addressed and TODO removed.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:778
+  auto LT = TLI->getTypeLegalizationCost(DL, DataTy);
+  ElementCount LVF = LT.second.getVectorElementCount();
+  if (!LVF.isScalable())
----------------
nit: can you give this a more intuitive name, like `LegalVF`?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:793
+      getMemoryOpCost(Opcode, VT->getElementType(), Alignment, 0, CostKind, I);
+  // NumGather * (NumElementsPerGather) * MemOpCost
+  return LT.first * (MaxNumVScale.getValue() * LVF.getKnownMinValue()) *
----------------
nit: `s/NumElementsPerGather/MaxNumElementsPerGather/`
(also, why the parentheses areound NumElementsPerGather?)


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:784
+  Optional<unsigned> MaxNumVScale = getMaxVScale();
+  // TODO: Replace assert for InstructionCost
+  assert(MaxNumVScale && "Expected valid max vscale value");
----------------
CarolineConcatto wrote:
> david-arm wrote:
> > nit: Sorry I didn't spot this before. Does the TODO still make sense here?
> Yes, because I cannot replace this to return InstructionCost without changing a big chain of functions to InstructionCost.
> I believe we will tackle this problem later, in another patch responsible to replace these functions to return InstructionCost.
For AArch64 there is always an architectural maximum, so getMaxVScale cannot return None. There isn't anything special we need to do such as returning an Invalid cost when we've already asserted that MaxVscale is not None (i.e. since the compiler will already have crashed if that wasn't the case, it doesn't really matter what happens after that point). So I agree that you can indeed remove the TODO as @david-arm suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93030