[PATCH] D147322: [AArch64] Improve fshl cost modeling if 3rd arg is constant.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I committed the patch on @zjaffal 's behalf as he is on vacation and this helps recover a regression we are seeing in our tracking.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:549
+      const auto *Entry =
+          CostTableLookup(FshlTbl, ICA.getID(), LegalisationCost.second);
+      if (Entry)
----------------
dmgreen wrote:
> You could just pass Intrinsic::fshl if the costs are expected to be the same, so long as there is a comment explaining it. It would help reduce the size of the table.
done in the committed version.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:564-569
+    if (RetTy->getScalarSizeInBits() == 32 ||
+        RetTy->getScalarSizeInBits() == 64)
+      Cost = 0; // fhsl for i32 and i64 can be lowered to a single extr
+                // instruction.
+    else if (HigherCost)
+      Cost = 1;
----------------
dmgreen wrote:
> I think this can just be `unsigned Cost = HigherCost ? 1 : 0`. The comment would be good to keep though.
> Maybe call it "ExtraCost" too, to show it's an addition not the base cost.
I tried that but it was causing some changes so I kept the original version for now. I updated the name.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/fshl.ll:4
 
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
 
----------------
dmgreen wrote:
> If there isn't already one, then adding a llvm.fshr version of this test file would be good too.
yep, there's `fhsr.ll` which is a copy of `fshl.ll` with fshl replaced by fshr.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/fshl.ll:236
+
+define i66 @fshl_i66(i66 %a, i66 %b) {
+; CHECK-LABEL: 'fshl_i66'
----------------
dmgreen wrote:
> Maybe add a test for i128 too? It might be relatively common from crypto functions and would help test one of the codepaths.
I added i128 tests in 8d4f92601c3c1fcc6ecc10352e2234b5c064e954


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147322