[PATCH] D143283: [AArch64][SVE]: custom lower AVGFloor/AVGCeil.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13402
+    APInt ApintTemp;
+    if (ISD::isConstantSplatVector(OpA.getOperand(1).getNode(), ApintTemp)) {
+      SDValue Add = DAG.getNode(ISD::ADD, dl, VT, OpA, OpB);
----------------
This is missing a check to ensure that the `and` value is a mask, and that the masked 'type' is smaller than the size of the current type.
Perhaps you can get some inspiration from `isExtendOrShiftOperand` which checks whether the operation is zero or sign-extended.

In either case, it would be nice to have this logic in separate `isSignedExtended()` and `isZeroExtended()` functions.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13405
+
+      if (Node->getOpcode() == ISD::AVGCEILU)
+        Add = DAG.getNode(ISD::ADD, dl, VT, Add, ConstantOne);
----------------
There are several uses of Node->getOpcode(), you can move that to a separate variable.

It would also be nice to have some variables:
* IsSigned (for AVGFLOORS and AVGCEILS)
* IsFloor (for AVGFLOORS and AVGFLOORU)
* ShiftOpc = IsSigned ? ISD::SRA : ISD::SRL;

That way you can simplify the code a bit, without having to check the opcodes everywhere, and you can combine some of the if/else branches.


================
Comment at: llvm/test/CodeGen/AArch64/sve-avg_floor_ceil.ll:4
+
+define <vscale x 2 x i64> @hadds_v2i64(<vscale x 2 x i64> %s0, <vscale x 2 x i64> %s1) {
+; CHECK-LABEL: hadds_v2i64:
----------------
Can you add these tests to the precursory patch, so that this patch only shows the diff?


================
Comment at: llvm/test/CodeGen/AArch64/sve-avg_floor_ceil.ll:1
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=aarch64-none-linux-gnu  -mcpu=neoverse-v1 | FileCheck %s
----------------
The filename has both a dash (`-`) and underscores (`_`). Can you rename it to sve-avg-floor-ceil.ll ?


================
Comment at: llvm/test/CodeGen/AArch64/sve-avg_floor_ceil.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=aarch64-none-linux-gnu  -mcpu=neoverse-v1 | FileCheck %s
+
----------------
Can you add two RUN lines, one for -mattr=+sve and one for -mattr=+sve2 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143283