[PATCH] D99699: [AArch64][SVE] Lowering sve.dot to DOT node

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis added a comment.

Thanks for the patch @junparser! Just a few minor comments from me. 🙂



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13342
             Dot.getOpcode() == AArch64ISD::SDOT) &&
-           ISD::isBuildVectorAllZeros(Dot.getOperand(0).getNode());
+           (isDupZero(Dot.getOperand(0)) ||
+            ISD::isBuildVectorAllZeros(Dot.getOperand(0).getNode()));
----------------
junparser wrote:
> dmgreen wrote:
> > Can we add a isZerosVector function that encapsulates isBuildVectorAllZeros and the splats/dups logic? It sounds generally useful to have a function that checks for the various ways a vector can be all zeros.
> > 
> > isConstantSplatVectorAllZeros may also be better, as it may already handle the ISD::SPLAT_VECTOR part.
> sound good to me, I'll update this.
Following on from this, it might be a good idea to write `isZerosVector` as:

```
static bool isZerosVector(const SDNode *N) {
  return ISD::isConstantSplatVectorAllZeros(N) || isConstantDupVectorAllZeros(N);
}
```

Might be a nicer separation of the logic here.

Your call, though!


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:2148-2150
+  // Look through a bit convert.
+  while (N->getOpcode() == ISD::BITCAST)
+    N = N->getOperand(0).getNode();
----------------
Might be better to do this _before_ the above `if(ISD::isConstantSplatVectorAllZeroes(N))`, in my opinion, to avoid running this loop twice (once in this function, once in `ISD::isConstantSplatVectorAllZeros`).


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:2152
+
+  if (N->getOpcode() != AArch64ISD::DUP && N->getOpcode() != ISD::SPLAT_VECTOR)
+    return false;
----------------
A cursory glance at `ISD::isConstantSplatVectorAllZeros` suggests that the SPLAT_VECTOR case is entirely captured earlier on, I think. I reckon you can get away with just:

```
if (N->getOpcode() != AArch64ISD::DUP)
  return false;
```




================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:2158-2160
+  if ((CINT && CINT->isNullValue()) || (CFP && CFP->isZero()))
+    return true;
+  return false;
----------------
nit:

```
return (CINT && CINT->isNullValue()) || (CFP && CFP->isZero());
```


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13339
   SDValue A = N->getOperand(1);
+
   // Handle commutivity
----------------
nit: superfluous newline.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-int-arith.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
----------------
The new `CHECK:` lines introduced look simple enough to write out by hand, so I would prefer to not use `utils/update_llc_checks.py` in this case because it has made the diff a lot noisier (e.g. all of the tests have been changed syntactically in this patch, but AFAICT none of them have changed functionally).

In this case, I think it should be sufficient to write your new tests like this:

```
define <vscale x 2 x i64> @test_sdot_i64_zero(<vscale x 2 x i64> %a, <vscale x 8 x i16> %b, <vscale x 8 x i16> %c) {
; CHECK-LABEL: test_sdot_i64_zero:
; CHECK:         sdot z0.d, z1.h, z2.h
; CHECK-NEXT:    ret
entry:
  %vdot1.i = call <vscale x 2 x i64> @llvm.aarch64.sve.sdot.nxv2i64(<vscale x 2 x i64> zeroinitializer, <vscale x 8 x i16> %b, <vscale x 8 x i16> %c)
  %ret = add <vscale x 2 x i64> %vdot1.i, %a
  ret <vscale x 2 x i64> %ret
}

define <vscale x 4 x i32> @test_sdot_i32_zero(<vscale x 4 x i32> %a, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c) {
; CHECK-LABEL: test_sdot_i32_zero:
; CHECK:         sdot z0.s, z1.b, z2.b
; CHECK-NEXT:    ret
entry:
  %vdot1.i = call <vscale x 4 x i32> @llvm.aarch64.sve.sdot.nxv4i32(<vscale x 4 x i32> zeroinitializer, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c)
  %ret = add <vscale x 4 x i32> %vdot1.i, %a
  ret <vscale x 4 x i32> %ret
}
```

... and:

```
define <vscale x 2 x i64> @test_udot_i64_zero(<vscale x 2 x i64> %a, <vscale x 8 x i16> %b, <vscale x 8 x i16> %c) {
; CHECK-LABEL: test_udot_i64_zero:
; CHECK:         udot z0.d, z1.h, z2.h
; CHECK-NEXT:    ret
entry:
  %vdot1.i = call <vscale x 2 x i64> @llvm.aarch64.sve.udot.nxv2i64(<vscale x 2 x i64> zeroinitializer, <vscale x 8 x i16> %b, <vscale x 8 x i16> %c)
  %ret = add <vscale x 2 x i64> %vdot1.i, %a
  ret <vscale x 2 x i64> %ret
}

define <vscale x 4 x i32> @test_udot_i32_zero(<vscale x 4 x i32> %a, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c) {
; CHECK-LABEL: test_udot_i32_zero:
; CHECK:         udot z0.s, z1.b, z2.b
; CHECK-NEXT:    ret
entry:
  %vdot1.i = call <vscale x 4 x i32> @llvm.aarch64.sve.udot.nxv4i32(<vscale x 4 x i32> zeroinitializer, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c)
  %ret = add <vscale x 4 x i32> %vdot1.i, %a
  ret <vscale x 4 x i32> %ret
}
```

... and then just revert all other changes in the file. :smile:


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

https://reviews.llvm.org/D99699