[AArch64][SVE] Remove Redundant aarch64.sve.convert.to.svbool

[MattDevereau]

Summary

Generated code resulted in redundant aarch64.sve.convert.to.svbool
calls for AArch64 Binary Operations. Narrow the more precise operands
instead of widening the less precise operands

[peterwaller-arm]

A couple of initial comments. It also looks like the code needs formatting.

I'm still thinking about the overall effect of the transform.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

388

This function looks strange to me. What use does the first element of the pair have, in terms of how it varies. When would it return anything other than a dyncast of Val for example? I think you can effectively inline this function and in my view it will make SolveRedundantSVBoolForBinOps clearer by removing the indirection of OptVal.first and OptVal.second.

393

For the comment, you could use an example to illustrate what is going on. It doesn't currently clearly explain the why or the what to me.

For the name, I don't think we have many things called 'solve', and I don't think of this operation as solving. I think it is a combine.

As a suggestion, tryCombineFromSVBoolBinOp may be an improvement?

395

A minor note on variable names: 'Opt' presumably stands for 'optional', but it's not a great name. What's lacking is that the name doesn't tell a reader what's in it. For example it's always Some(Value) in the body of the if, and never None, so it's not really optional, in fact it's the pair. But naming this Pair wouldn't be great either, since that wouldn't give any information to the reader about what's in it. If I find myself using such variable names it is usually a hint something can be improved. As suggested above, I think the optional pair can be removed entirely by inlining GetPredicatedBinOp.

487

The logic of this function could be clearer. EarliestReplacement is a detail of the first combine, when it succeeds. Combines which follow this should not refer to that detail, otherwise it is suggestive that these combines are related.

I think in this case the function should be structured like so:

instCombineConvertFromSVBool() {
  // ... try first combine ...
  // ... try second combine ...
  return None;
}

So that the code relating to an individual combine does not span other combines (as caused by the return statement here).

The main benefit of my suggestion is that further combines can be added without changing existing code.

[MattDevereau]

Removed GetPredicatedBinOp function and inlined it
Updated tryCombineFromSVBoolBinOp comment and function name
ran clang format

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

388

My logic here was that I need to check that the initial convert.from.svbool operand is both an intrinsic and a binop intrinsic so I may as well return both pieces of work neatly in this helper function which might grow as we add more binops to the list. If you don't think this its worth it then inlining it is fine

393

I'm not sure how I can give an example of what the code is addressing without pasting in the test i've added or adding a really descriptive comment that is harder to read than an example. I've changed it a bit to hopefully make it more clear.

Changing the function name sounds good.

395

I've removed the pair now

487

I've moved the tryCombineFromSVBoolBinOp call further up the function now

[DavidTruby]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

406

nit: you don't need the <> here it can just be IRBuilder

[MattDevereau]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

406

i'm compiling this locally with clang++11 with default flags, it errors when this is removed

[MattDevereau]

Added tests for other vector types and updated function comment

[DavidTruby]

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

406

Interesting! clang 13 and gcc also fail to compile without them, but it works with C++17 instead of 14. I wonder why the empty brackets are necessary here... Anyway they do seem to be so ignore my comment :)

[MattDevereau]

An offline discussion prompted a rewording of the function's comment

[MattDevereau]

Add SVE intrinsics BIC, EOR, NAND, NOR, ORN, ORR to
tryCombineFromSVBoolBinOp, allowing them to remove
redundant convert.to.svbool calls

[MattDevereau]

added other binops

[peterwaller-arm]

LGTM with some editorial suggestions.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

380–395

Heads up, LLVM doesn't use c-style /* */ comments. https://llvm.org/docs/CodingStandards.html#comment-formatting

I've had a go at editing the text here with the goal of simplifying it.

415

You can elide all the break; here aside from the last one to shorten this switch a little.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp

for (unsigned I = 0; I < PN->getNumIncomingValues(); I++) {
     NPN->addIncoming(Reinterpret->getOperand(0), PN->getIncomingBlock(I));
     Worklist.push_back(Reinterpret);
   }
 
   // Cleanup Phi Node and reinterprets
   return IC.replaceInstUsesWith(II, NPN);
 }
 
+//(from_svbool (binop (to_svbool pred) (_) (_)))) =>
+// (binop (pred) (from_svbool _) (from_svbool _))
+
+// Functions which call a predicated binary operation where the type width for
+// the predicate is narrower than the two operands type widths and then returns
+// the result with the predicate width results in redundant operations being
+// generated. When widening the predicate type to the operand types with
+// to.svbool, "ptrue" and "and" instructions are generated as the "new" lanes
+// generated by to.svbool must be zeroed. These zeroed lanes are redundant as
+// binary operations such as "and" are operationally equivalent regardless of
+// operand or predicate width, and the "new" lanes are ignored when narrowing
+// the binary operation result with from.svbool to satisfy the return width.
+
+// Instead, narrow the operand type widths to the predicate type width with
+// from.svbool as this is a nop and will not generate additional instructions.
+static Optional<Instruction *> tryCombineFromSVBoolBinOp(InstCombiner &IC,
+                                                         IntrinsicInst &II) {
+  auto BinOp = dyn_cast<IntrinsicInst>(II.getOperand(0));
+  if (!BinOp)
+    return None;
+
+  auto IntrinsicID = BinOp->getIntrinsicID();
+  switch (IntrinsicID) {
+  case Intrinsic::aarch64_sve_and_z:
+    break;
+  default:
+    return None;
+  }
+
+  auto BinOpPred = BinOp->getOperand(0);
+  auto BinOpOp1 = BinOp->getOperand(1);
+  auto BinOpOp2 = BinOp->getOperand(2);
+
+  auto PredIntr = dyn_cast<IntrinsicInst>(BinOpPred);
+  if (!PredIntr ||
+      PredIntr->getIntrinsicID() != Intrinsic::aarch64_sve_convert_to_svbool)
+    return None;
+
+  auto PredOp = PredIntr->getOperand(0);
+  auto PredOpTy = cast<VectorType>(PredOp->getType());
+  if (PredOpTy != II.getType())
+    return None;
+
+  IRBuilder<> Builder(II.getContext());
+  Builder.SetInsertPoint(&II);
+
+  SmallVector<Value *> NarrowedBinOpArgs = {PredOp};
+  auto NarrowBinOpOp1 = Builder.CreateIntrinsic(
+      Intrinsic::aarch64_sve_convert_from_svbool, {PredOpTy}, {BinOpOp1});
+  NarrowedBinOpArgs.push_back(NarrowBinOpOp1);
+  if (BinOpOp1 == BinOpOp2)
+    NarrowedBinOpArgs.push_back(NarrowBinOpOp1);
+  else
+    NarrowedBinOpArgs.push_back(Builder.CreateIntrinsic(
+        Intrinsic::aarch64_sve_convert_from_svbool, {PredOpTy}, {BinOpOp2}));
+
+  auto NarrowedBinOp =
+      Builder.CreateIntrinsic(IntrinsicID, {PredOpTy}, NarrowedBinOpArgs);
+  return IC.replaceInstUsesWith(II, NarrowedBinOp);
+}
+
 static Optional<Instruction *> instCombineConvertFromSVBool(InstCombiner &IC,
                                                             IntrinsicInst &II) {
   // If the reinterpret instruction operand is a PHI Node
   if (isa<PHINode>(II.getArgOperand(0)))
     return processPhiNode(IC, II);
 
+  if (auto BinOpCombine = tryCombineFromSVBoolBinOp(IC, II))
+    return BinOpCombine;
+
   SmallVector<Instruction *, 32> CandidatesForRemoval;
   Value *Cursor = II.getOperand(0), *EarliestReplacement = nullptr;
 
   const auto *IVTy = cast<VectorType>(II.getType());
 
   // Walk the chain of conversions.
   while (Cursor) {
     // If the type of the cursor has fewer lanes than the final result, zeroing
static Optional<Instruction *> instCombineConvertFromSVBool(InstCombiner &IC,
   }
 
   // If no viable replacement in the conversion chain was found, there is
   // nothing to do.
   if (!EarliestReplacement)
     return None;
 
   return IC.replaceInstUsesWith(II, EarliestReplacement);
 }
 
 static Optional<Instruction *> instCombineSVEDup(InstCombiner &IC,
                                                  IntrinsicInst &II) {
   IntrinsicInst *Pg = dyn_cast<IntrinsicInst>(II.getArgOperand(1));
   if (!Pg)
     return None;
 
   if (Pg->getIntrinsicID() != Intrinsic::aarch64_sve_ptrue)

llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-to-svbool-binops.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -instcombine < %s | FileCheck %s
+
+target triple = "aarch64-unknown-linux-gnu"
+
+define <vscale x 4 x i1> @try_combine_svbool_binop_and_0(<vscale x 4 x i1> %a, <vscale x 16 x i1> %b, <vscale x 16 x i1> %c) {
+; CHECK-LABEL: @try_combine_svbool_binop_and_0(
+; CHECK-NEXT:    [[TMP1:%.*]] = call <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1> [[B:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1> [[C:%.*]])
+; CHECK-NEXT:    [[TMP3:%.*]] = call <vscale x 4 x i1> @llvm.aarch64.sve.and.z.nxv4i1(<vscale x 4 x i1> [[A:%.*]], <vscale x 4 x i1> [[TMP1]], <vscale x 4 x i1> [[TMP2]])
+; CHECK-NEXT:    ret <vscale x 4 x i1> [[TMP3]]
+;
+  %t1 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv4i1(<vscale x 4 x i1> %a)
+  %t2 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.and.z.nxv16i1(<vscale x 16 x i1> %t1, <vscale x 16 x i1> %b, <vscale x 16 x i1> %c)
+  %t3 = tail call <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1> %t2)
+  ret <vscale x 4 x i1> %t3
+}
+
+define <vscale x 8 x i1> @try_combine_svbool_binop_and_1(<vscale x 8 x i1> %a, <vscale x 16 x i1> %b) {
+; CHECK-LABEL: @try_combine_svbool_binop_and_1(
+; CHECK-NEXT:    [[TMP1:%.*]] = call <vscale x 8 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv8i1(<vscale x 16 x i1> [[B:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call <vscale x 8 x i1> @llvm.aarch64.sve.and.z.nxv8i1(<vscale x 8 x i1> [[A:%.*]], <vscale x 8 x i1> [[TMP1]], <vscale x 8 x i1> [[TMP1]])
+; CHECK-NEXT:    ret <vscale x 8 x i1> [[TMP2]]
+;
+  %t1 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv8i1(<vscale x 8 x i1> %a)
+  %t2 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.and.z.nxv16i1(<vscale x 16 x i1> %t1, <vscale x 16 x i1> %b, <vscale x 16 x i1> %b)
+  %t3 = tail call <vscale x 8 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv8i1(<vscale x 16 x i1> %t2)
+  ret <vscale x 8 x i1> %t3
+}
+
+define <vscale x 4 x i1> @try_combine_svbool_binop_and_2(<vscale x 4 x i1> %a, <vscale x 16 x i1> %b) {
+; CHECK-LABEL: @try_combine_svbool_binop_and_2(
+; CHECK-NEXT:    [[TMP1:%.*]] = call <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1> [[B:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call <vscale x 4 x i1> @llvm.aarch64.sve.and.z.nxv4i1(<vscale x 4 x i1> [[A:%.*]], <vscale x 4 x i1> [[TMP1]], <vscale x 4 x i1> [[TMP1]])
+; CHECK-NEXT:    ret <vscale x 4 x i1> [[TMP2]]
+;
+  %t1 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv4i1(<vscale x 4 x i1> %a)
+  %t2 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.and.z.nxv16i1(<vscale x 16 x i1> %t1, <vscale x 16 x i1> %b, <vscale x 16 x i1> %b)
+  %t3 = tail call <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1> %t2)
+  ret <vscale x 4 x i1> %t3
+}
+
+define <vscale x 2 x i1> @try_combine_svbool_binop_and_3(<vscale x 2 x i1> %a, <vscale x 16 x i1> %b) {
+; CHECK-LABEL: @try_combine_svbool_binop_and_3(
+; CHECK-NEXT:    [[TMP1:%.*]] = call <vscale x 2 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv2i1(<vscale x 16 x i1> [[B:%.*]])
+; CHECK-NEXT:    [[TMP2:%.*]] = call <vscale x 2 x i1> @llvm.aarch64.sve.and.z.nxv2i1(<vscale x 2 x i1> [[A:%.*]], <vscale x 2 x i1> [[TMP1]], <vscale x 2 x i1> [[TMP1]])
+; CHECK-NEXT:    ret <vscale x 2 x i1> [[TMP2]]
+;
+  %t1 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv2i1(<vscale x 2 x i1> %a)
+  %t2 = tail call <vscale x 16 x i1> @llvm.aarch64.sve.and.z.nxv16i1(<vscale x 16 x i1> %t1, <vscale x 16 x i1> %b, <vscale x 16 x i1> %b)
+  %t3 = tail call <vscale x 2 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv2i1(<vscale x 16 x i1> %t2)
+  ret <vscale x 2 x i1> %t3
+}
+
+declare <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv8i1(<vscale x 8 x i1>)
+declare <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv4i1(<vscale x 4 x i1>)
+declare <vscale x 16 x i1> @llvm.aarch64.sve.convert.to.svbool.nxv2i1(<vscale x 2 x i1>)
+declare <vscale x 16 x i1> @llvm.aarch64.sve.and.z.nxv16i1(<vscale x 16 x i1>, <vscale x 16 x i1>, <vscale x 16 x i1>)
+declare <vscale x 8 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv8i1(<vscale x 16 x i1>)
+declare <vscale x 4 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv4i1(<vscale x 16 x i1>)
+declare <vscale x 2 x i1> @llvm.aarch64.sve.convert.from.svbool.nxv2i1(<vscale x 16 x i1>)
+
+attributes #0 = { "target-features"="+sve" }