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

[mgabka via Phabricator via llvm-commits]

mgabka added a comment.

I think it would be good to explain in the commit message why conversion to _u is a good thing, otherwise it is not clear why we are doing it.



================
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);
+  }
----------------
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.


================
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:
----------------
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


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-m-to-x.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
----------------
I am not a big fan of the name of this file, it suggests that there should be intrinsics with _x or _m suffix being used but there no such ones.

I think it would also be good to sort the test in alphabetical order by the name of the intrinsic being tested, that would make navigation through this long file easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005