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

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 20 10:19:28 PDT 2022


dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

Whichever way you go with the test, I think we can all agree that the change looks fine. LGTM



================
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))
----------------
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.


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

https://reviews.llvm.org/D127680



More information about the llvm-commits mailing list