[PATCH] D152005: [SVE ACLE] Implement IR combines to convert intrinsics used for _m C/C++ builtins

[Jolanta Jensen via Phabricator via llvm-commits]

jolanta.jensen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1793
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_mls_u);
+  case Intrinsic::aarch64_sve_mul: {
+    if (auto MUL_U =
----------------
mul and mul_u execute identical code. Please advise if this can be solved some better way.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1338-1342
+  SmallVector<Value *> Args(II.arg_size(), nullptr);
+  Args.clear();
+  for (Value *V : II.args()) {
+    Args.push_back(V);
+  }
----------------
mgabka wrote:
> this isn't efficient (as you are copying element by element and doing unnecessary clear), and it is a lot of line of code, SmallVector has a dedicated constructor which can take an iterator_range and create a vector from it, please use it instead.
Fixed.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1715-1774
+  case Intrinsic::aarch64_sve_fabd:
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fabd_u);
+  case Intrinsic::aarch64_sve_fdiv:
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fdiv_u);
+  case Intrinsic::aarch64_sve_fmax:
+    return instCombineSVEAllActive(IC, II, Intrinsic::aarch64_sve_fmax_u);
+  case Intrinsic::aarch64_sve_fmaxnm:
----------------
jolanta.jensen wrote:
> mgabka wrote:
> > would be good if the order of intrinsics here aligned with the order in the test file so it would be easier to check that we have full coverage
> I think they are. But I could make a mistake, I'll check.
Should be now in the same order as in the test file. And the test file is renamed and alphabetically ordered within the sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005