[PATCH] D127680: [BasicTTI] Allow generic handling of scalable vector fshr/fshl

Mon Jun 20 10:41:33 PDT 2022

reames added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:425
+  case Intrinsic::fshr:
+    // Match legacy behavior - this is probably not the right costing here
+    if (isa<ScalableVectorType>(RetTy))
----------------
dmgreen wrote:
> reames wrote:
> > dmgreen wrote:
> > > craig.topper wrote:
> > > > Seems like this should be a FIXME?
> > > I don't think this needs to be added. Just updating the costs in the test files to whatever is now produced will likely be more accurate, and more in-line with the scalar and fixed length vector costs. We can then update them more accurately in the future.
> > I don't work on SVE, don't have any sense for what "reasonable" costs are for the target, and don't particularly want to be on the blame list for a potential performance swing investigation.  If you think the costs are likely reasonable, I'd ask that you remove the bailout in a separate following change.
> The numbers looked fine when I looked at them. 13 IIRC? It would be unreasonable for someone to blame this commit for performance regression, considering how little costs we have for funnel shifts. It's more likely to make things better, and they are free to blame me if it does make things worse.
> 
> It would seem simpler to remove this code and just update the test. I believe that was what @david-arm was asking for. But if you insist then sure, I can update afterwards. That should be simple enough.
In this particular case, I'm probably being paranoid, but in generally, yes I think it's well worth splitting changes to minimize the impact on other targets and configurations.  You say no one would reasonable blame this patch for a performance regression.  My experience says I've wasted a lot of time whether someone was reasonable on patches just this simple.  :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127680