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

[Dave Green via Phabricator via llvm-commits]

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

Thanks for taking this over and doing the updates. There are a few extra suggestions inline but otherwise LGTM.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:549
+      const auto *Entry =
+          CostTableLookup(FshlTbl, ICA.getID(), LegalisationCost.second);
+      if (Entry)
----------------
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.


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


================
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"
 
----------------
If there isn't already one, then adding a llvm.fshr version of this test file would be good too.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147322