[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:13392
+  EVT VT = Op.getValueType();
+  SDValue ConstantOne = DAG.getConstant(1, dl, VT);
+
----------------
Move this closer to use.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13395
+  bool IsCeil = (Op->getOpcode() == ISD::AVGCEILS || Op->getOpcode() == ISD::AVGCEILU);
+
+  bool IsSigned = (Op->getOpcode() == ISD::AVGFLOORS || Op->getOpcode() == ISD::AVGCEILS);
----------------
nit: unnecessary newline.

Also, these lines exceed the 80char limit, so please run clang-format on this code.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13408
+  auto IsSignExtended = [&DAG](SDValue &Node){
+    if (Node.getOpcode() != ISD::SIGN_EXTEND_INREG)
+        return false;
----------------
You can remove this condition, it's covered by DAG.ComputeNumSignBits.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13414-13430
+  if (!IsSigned && IsZeroExtended(OpA) && IsZeroExtended(OpB)) {
+    // if the Ops are already extended, then shift their addition directly,
+    // it's pointless to shift each on on its own.
+    SDValue Add = DAG.getNode(ISD::ADD, dl, VT, OpA, OpB);
+    if (IsCeil)
+      Add = DAG.getNode(ISD::ADD, dl, VT, Add, ConstantOne);
+    return DAG.getNode(ISD::SRL, dl, VT, Add, ConstantOne);
----------------
You can combine these cases doing something like this:

  if ((IsSigned && IsSignExtended(OpA) && IsSignExtended(OpB)) ||
      (!IsSigned && IsZeroExtended(OpA) && IsZeroExtended(OpB))) {
    ... 
    unsigned ShiftOpc = IsSigned ? ISD::SRA : ISD::SRL;
    return DAG.getNode(ShiftOpc, dl, VT, Add, ConstantOne);
  }


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13436-13440
+  SDValue tmp;
+  if (IsCeil)
+    tmp = DAG.getNode(ISD::OR, dl, VT, OpA, OpB);
+  else
+    tmp = DAG.getNode(ISD::AND, dl, VT, OpA, OpB);
----------------
nit: SDValue Tmp = DAG.getNode(IsCeil ? ISD::OR : ISD::AND, dl, VT, OpA, OpB);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143283