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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks for taking this over.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:552
+    int64_t NumElements = 1;
+    if (RetTy->isIntegerTy())
+      break;
----------------
Is this correct, that it's breaking if it is an integer type?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:559
+                       RetTy->getScalarSizeInBits() != 64 &&
+                       RetTy->getScalarSizeInBits() < 64) ||
+                      (RetTy->getScalarSizeInBits() % 64 != 0);
----------------
`!= 64` is covered by `< 64`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:568
+      Cost = 2;
+      dbgs() << "Cost: " << TyL.first * Cost << "\n";
+    } else
----------------
There is a debug message left here, which is coming out in the tests too.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:572
+
+    return TyL.first * Cost * NumElements;
+  }
----------------
Do you have any opinions of whether this should be `TyL.first * Cost` (with a Cost of 1/2) or `TyL.first + Cost` (with a cost of 0/1)? It might capture the "extra shift" a little better, for integer types at least.
As far as I can see NumElements is always 1 here?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/fshl.ll:234
+entry:
+  %fshl = tail call <2 x i66> @llvm.fshl.v4i66(<2 x i66> %a, <2 x i66> %b, <2 x i66> <i66 1, i66 2>)
+  ret <2 x i66> %fshl
----------------
Should this be with equal operands and i66? Otherwise it would not get past the isUniform check. Or is it intended to check the other code?
We are probably getting into pretty uncommon cases here either way :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147322