[PATCH] D99074: [llvm][AArch64][SVE] Fold literals into math instructions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1820-1823
+multiclass sve_intrinsic_compact_fp_immediates<string I, string IX,
+                  ValueType itype, ValueType ftype, ValueType sftype,
+                  SDPatternOperator op, SDPatternOperator ir_op, FPImmLeaf A,
+                  int imm> {
----------------
Assuming my other comments are valid I believe we won't need a multiclass and can then follow the same idiom as with `class SVE_2_Op_Pred...` (i.e. add new classes in the `SVE pattern match helpers` section towards the start of this file.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1824-1827
+  def : Pat<(ftype (op (itype PPR_3b:$Pg),
+                          (ftype ZPR:$Zs1),
+                          (ftype (AArch64dup (sftype A))))),
+            (!cast<Instruction>(I) PPR_3b:$Pg, ZPR:$Zs1, imm)>;
----------------
This pattern seems like a better fit for the `sve_fp_2op_i_p_zds` multiclass where the relevant instructions are defined.  Is that possible?


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1828-1831
+  def : Pat<(ftype (ir_op (itype (AArch64ptrue 31)),
+                            (ftype ZPR:$Zs1),
+                            (ftype (AArch64dup (sftype A))))),
+            (!cast<Instruction>(IX) (PTRUE_S 31), ZPR:$Zs1, imm)>;
----------------
Is this pattern necessary?  I'm thinking that even for the IR ops you can just pass the predicate through and thus we can reuse the first pattern.  Doing this means we'll also take advantage of the immediate forms when lowering VLS operations.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1835
+
+multiclass sve_fp_2op_i_p_zds_zx<string I, Operand imm_ty, FPImmLeaf A, FPImmLeaf B, SDPatternOperator op, SDPatternOperator ir_op = null_frag> {
+  def _UNDEF_H : PredTwoOpImmPseudo<NAME # _H, ZPR16, imm_ty, FalseLanesUndef>;
----------------
There's a bit of clash with naming here.  In the past `_zx` implied zeroing where as we now typically attach element types info for the undef case (i.e. `_hfd` in this instance) and then `zeroing_hfd` for the zeroing case.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1859
+
+multiclass sve_fp_2op_i_p_zds_zx_zeroing<string IZ, Operand imm_ty, FPImmLeaf A, FPImmLeaf B, SDPatternOperator op> {
+  def _ZERO_H : PredTwoOpImmPseudo<NAME # _H, ZPR16, imm_ty, FalseLanesZero>;
----------------
Perhaps you don't need this parameter.  Typically you can reference `NAME` which is the string after the `defm` when used.  So for example you have:
```
defm FADD_ZPZI    : sve_fp_2op_i_p_zds_zx_zeroing
```
so `NAME` will be `FADD_ZPZI` and thus you can then do:
```
defm : sve_intrinsic_compact_fp_immediates_zeroing<NAME # "_ZERO_H",
```
This suggestion likely applies to `sve_intrinsic_compact_fp_immediates` also, especially if the second pattern can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99074