[PATCH] D84548: [AArch64][SVE] Add lowering for llvm fceil

Fri Jul 24 13:42:15 PDT 2020

efriedma added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3444
+  SDLoc DL(Op);
+  auto NewOperand = SDValue(DAG.getMachineNode(TargetOpcode::IMPLICIT_DEF, DL, VT), 0);
+  return DAG.getNode(Op.getOpcode(), DL, VT, NewOperand, Op->getOperand(0));
----------------
We shouldn't be calling getMachineNode in legalization.  The way to get an uninitialized value before isel is getUNDEF().

Also, this looks like it's creating an FCEIL with two operands.  That's a bad idea; we have a bunch of assertions in getNode() to ensure SelectionDAG nodes are well-formed.  Even if those assertions don't catch this issue right now, they might in the future.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:107
+  //Uniary Floating Point Operation
+  FRINTP_PRED,
+
----------------
This doesn't match the naming convention we're using for these opcodes.  See the comment at the beginning of this file: according to those rules, this should be named FRINTP_MERGE_PASSTHRU.

But really, probably better to actually implement FRINTP_PRED, without the extra operand.  Maybe try looking at https://reviews.llvm.org/D83765 ?

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

https://reviews.llvm.org/D84548