[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:1331-1350
+instCombineSVEAllActive2VA(InstCombiner &IC, IntrinsicInst &II) {
+  auto *OpPredicate = II.getOperand(0);
+  auto *OpA = II.getOperand(1);
+  auto *OpB = II.getOperand(2);
+  if (!match(OpPredicate, m_Intrinsic<Intrinsic::aarch64_sve_ptrue>(
+                              m_ConstantInt<AArch64SVEPredPattern::all>())))
+    return std::nullopt;
----------------
paulwalker-arm wrote:
> Rather than have nested switch statements perhaps you can pass the new `IntrinsicID` into this function?  
> 
> Since both this function and `instCombineSVEAllActive3VA` don't change the original call's operands, I think you should be able to make use of II.args().  I'm not sure if there's a variant of `CreateIntrinsic` that'll take this directly but if not, you should be able to create a `SmallVector` from it and pass that to `CreateIntrinsic`.
Fixed. So much better now :)


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1338-1339
+    return std::nullopt;
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+  switch (II.getIntrinsicID()) {
----------------
paulwalker-arm wrote:
> `InstCombiner` comes with its own `IRBuilder` already set up.  It should be as simple as replacing your new `Builder.Create...`lines with `IC.Builder.Create...`.  You'll see the existing SVE functions have been updated similarly.
Changed to InstCombiner own IRBuilder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005