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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3545
+  case ISD::FCEIL:
+    return LowerToPredicatedOp(Op, DAG, AArch64ISD::FRINTP_PRED, true);
   case ISD::SMIN:
----------------
To make the relationship clearer between the original nodes and their predicated counterparts we just add a suffix.  So in this case the predicated node should be named FCEIL_PRED.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15220
   SmallVector<SDValue, 4> Operands = {Pg};
+  if(Merge)
+    Operands.insert(Operands.begin(), DAG.getUNDEF(VT));
----------------
I don't understand why you need this change.  By definitions the _PRED nodes should take the form:

ISDNODE Op1, Op2...OpN -> ISDNODE_PRED Pg, Op1, Op2...OpN

I would expect this function to do what's required without any changes. I suspect any issues are likely down to mistakes within the isel patterns.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:872
   SDValue LowerToPredicatedOp(SDValue Op, SelectionDAG &DAG,
-                              unsigned NewOp) const;
+                              unsigned NewOp, bool Merge = false) const;
   SDValue LowerEXTRACT_SUBVECTOR(SDValue Op, SelectionDAG &DAG) const;
----------------
Merge is not required, see comment on function definition.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:165
 
+def SDT_AArch64UnaryOp : SDTypeProfile<1, 1, [SDTCisVec<0>, SDTCisVec<1>]>;
+def AArch64frintp_p : SDNode<"AArch64ISD::FRINTP_PRED", SDT_AArch64UnaryOp>;
----------------
This is incorrect because the unary _PRED nodes should have 2 operands. The predicated followed by the data operand. See SDT_AArch64Arith for inspiration, where you just need to drop the stuff related to Op3.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:2270-2271
 
-multiclass sve_fp_2op_p_zd_HSD<bits<5> opc, string asm, SDPatternOperator op> {
+multiclass sve_fp_2op_p_zd_HSD<bits<5> opc, string asm, SDPatternOperator op,
+                                                SDPatternOperator op2 = null_frag> {
   def _H : sve_fp_2op_p_zd<{ 0b01, opc }, asm, ZPR16, ZPR16, ElementSizeH>;
----------------
Happy as is, but you could enhance readability if the operators are named op_merge and op_pred.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:2281
+  def : Pat<(nxv8f16 (op2 nxv8f16:$Op1)),
+            (!cast<Instruction>(NAME # _H) (IMPLICIT_DEF), (PTRUE_H 31), $Op1)>;
+  def : Pat<(nxv4f32 (op2 nxv4f32:$Op1)),
----------------
There should be no need to explicitly create the PTRUE here because the _PRED nodes already provide the predicate (which will be Op1 when LowerToPredicatedOp is restored) that should be used directly.

To be honest I'm not entirely sure how this even works since when creating FRINTP_PRED you currently set Op1 to DAG.getUNDEF().

There should also be patterns for the other legal floating point MVTs (i.e. nxv2f16, nxv4f16 and nxv2f32).


================
Comment at: llvm/test/CodeGen/AArch64/sve-fp.ll:413
+
+define <vscale x 8 x half> @frintp_nxv8f16(<vscale x 8 x half> %a) {
+; CHECK-LABEL: frintp_nxv8f16:
----------------
You'll need tests for the other legal floating point MVTs (i.e. nxv2f16, nxv4f16 and nxv2f32).


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

https://reviews.llvm.org/D84548