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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:531
+    // TODO: Add handling for fshl where third argument is not a constant.
+    // TODO: Also use for fshr.
+    const TTI::OperandValueInfo OpInfoZ = TTI::getOperandInfo(ICA.getArgs()[2]);
----------------
I would just combine fshr into a single patch I think. They seem to be essentially the same.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:539
+      static const CostTblEntry FshlTbl[] = {
+          {Intrinsic::fshl, MVT::v4i32, 3}, // ushr + shl + orr
+          {Intrinsic::fshl, MVT::v2i64, 3},
----------------
Can you add i16 and i8 vector types too? And 64bit vector sizes like v2i32.
Can you add a FIXME that the cost could be lower if the codegen was better.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:548-549
+
+    Type *ArgTy = ICA.getArgs()[0]->getType();
+    auto TyL = getTypeLegalizationCost(ArgTy);
+
----------------
Why does this not use RetTy, as above?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:551
+
+    if (!ArgTy->isIntegerTy())
+      break;
----------------
What is this guarding against? Other vector types?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:561
+                // instruction.
+    else if (ArgTy->getScalarSizeInBits() == 8 ||
+             ArgTy->getScalarSizeInBits() == 16)
----------------
I don't thinks its "equal to i8 or i16", it should be "not equal to i32 or i64", and any smaller sizes should get a larger cost. Or any larger sizes not a multiple of 64.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/fshl.ll:199
 entry:
   %fshl = tail call <2 x i64> @llvm.fshl.v4i64(<2 x i64> %a, <2 x i64> %b, <2 x i64> <i64 1, i64 1>)
   ret <2 x i64> %fshl
----------------
v2i64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147322