[PATCH] D92094: [CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D92094#2422952 <https://reviews.llvm.org/D92094#2422952>, @ctetreau wrote:

>> I'd prefer if we didn't knowingly create broken paths here
>
> It is unfortunate, but this is all fallout from the VectorType refactor. All of this code was calling `getNumElements()` without checking `isScalable()`. The general approach for doing the VectorType refactor was "if it unconditionally calls `getNumElements`, make it unconditionally cast to `FixedVectorType`. If it was broken before, it will still be broken now, but at least we'll get an assertion failure instead of it just silently miscompiling"
>
> Unfortunately, that means that a great deal of this TTI stuff is broken for scalable vectors, since it just wasn't being exercised.

That's right, the paths are already broken. This patch merely tries to fix up a few code-paths that specifically don't require `FixedVectorType`. In this case, only the scalarization strictly requires `FixedVectorType` in this function. Other code-paths fail due to the called functions not yet working for scalable vectors, but those should be fixed in separate patches (e.g. one patch to address reductions in `getTypeBasedIntrinsicInstrCost`, one for gathers/scatters in `getGatherScatterOpCost` and one that fixes `getArithmeticInstrCost` for the codepath exercised by fshl/fshr).

> Once D91174 <https://reviews.llvm.org/D91174> is merged, I think it would be useful if somebody went through and updated the default TTI stuff to just return an invalid cost before any unconditional cast to `FixedVectorType`. At this point, we can get the stub of the comprehensive scalable vector TTI test, that initially just checks that most things return an invalid cost. (A few things do currently work)

Yes we should do that. From having seen how this work is layered, it will be quite a substantial task, as it requires changing the interfaces of the TTI functions and all its callers. We already started to change some of the 'root' nodes of the callgraph in e.g. D92178 <https://reviews.llvm.org/D92178> (LoopVectorize), D92238 <https://reviews.llvm.org/D92238> (SCEVExpander) and the SLPVectorizer, as those changes are NFC until any of the TTI functions returns an Invalid, and we can already have it do the sensible thing (i.e. fallback to "not vectorize"). The TTI methods themselves should either propagate the Invalid state (which hopefully comes for free), or as you suggested, return an Invalid cost. When the TTI functions start returning Invalid, we'll need to add additional tests for the Vectorizer and SLPVectorizer to guard they're indeed doing the right thing.



================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1280
 
-    if (VF.isVector() && !RetTy->isVoidTy())
-      RetTy = FixedVectorType::get(RetTy, VF.getKnownMinValue());
+    // TODO: Handle the remaining intrinsic with scalable vector type
+    if (isa<ScalableVectorType>(RetTy))
----------------
samparker wrote:
> This can be hoisted above the loop, which is makes it a bit more clear that we're not handled scalarization here.
+1


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1284
+
+    if (VF.isVector() && !RetTy->isVoidTy()) {
+      RetTy = VectorType::get(RetTy, VF);
----------------
samparker wrote:
> nit: no need for brackets and shouldn't this just be kept as FixedVectorType anyway?
+1


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-getIntrinsicInstrCost-cctz-ctlz.ll:14
+; CHECK-LABEL: 'ctlz_nxv4i32'
+; CHECK-NEXT: Cost Model: Found an estimated cost of {{[0-9]+}} for instruction:
+; CHECK-NETX: Cost Model: Found an estimated cost of {{[0-9]+}} for instruction:
----------------
even though the cost doesn't directly matter much, it may be useful to just fill in the value it uses currently, so that when we improve the cost-model for scalable vectors, we can see the changes we make.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92094