[PATCH] D137411: [AArch64][SVE2] Add the SVE2.1 logical quadword reduction instructions

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:9208
+    : I<(outs V128:$Vd), (ins PPR3bAny:$Pg, zpr_ty:$Zn),
+        mnemonic, "\t$Vd." # vec_sfx # ", $Pg, $Zn",
+        "", []>, Sched<[]> {
----------------
david-arm wrote:
> paulwalker-arm wrote:
> > Not your fault but this is a shame because otherwise we could just extend `sve_int_reduce`.
> > 
> > With that said, are you able to follow the same idiom as `sve_int_reduce` for the `QV` reductions because your current direction will mean a separate instruction class per type of reduction and I cannot immediately see a need for that.
> Given they are quite different encodings does this really make sense?
> 
> These three encoding groups all differ by 3 bits on top of the opc
> SVE bitwise logical reduction (quadwords): bits 21-19: 011
> SVE integer add reduction (quadwords): bits 21-19: 000
> SVE integer min/max reduction (quadwords): bits 21-19: 001
> 
> To be honest I'm quite confused by how the existing sve_int_reduce hierarchy is written. For example, what does the _1 in sve_int_reduce_1 mean and how does that relate to the instructions? Is it just a random suffix? Personally, if we have to go down this route I'd prefer something more meaningful than just copying how we've done this before. Would this be ok?
> 
> defm EORQV_VPZ : sve2p1_log_int_reduce_q<...>;
> ...
> defm ADDQV_VPZ : sve2p1_arith_int_reduce_q<...>;
> ...
> defm SMAXQV_VPZ : sve2p1_minmax_int_reduce_q<...>;
> 
> where the multiclasses look like
> 
> multiclass sve2p1_log_int_reduce_q<bits<2> opc, string mnemonic> {
>   def _B : sve2p1_int_reduction_q<0b00, opc, 0b011, mnemonic, ZPR8,  "16b">;
>   def _H : sve2p1_int_reduction_q<0b01, opc, 0b011, mnemonic, ZPR16, "8h">;
>   def _S : sve2p1_int_reduction_q<0b10, opc, 0b011, mnemonic, ZPR32, "4s">;
>   def _D : sve2p1_int_reduction_q<0b11, opc, 0b011, mnemonic, ZPR64, "2d">;
> }
> 
> where 0b011 refers to bits 21-19 of the encoding group.
>From what I can see all the quadword reductions (e.g. SMAXQV) differ by a single bit (18) when compared to their equivalent predicated reductions (e.g. SMAXV). Given this I don't see any reason why you should not use the same structure.

In this instance I'm not asking you to reuse any existing classes (that would be amazing but the current parsing of NEON registers precludes it) so I guess I'm just saying you can copy `sve_int_reduce` as `sve_int_reduce_quadwards` and then create the three multiclasses as you've described above.

Please go with whatever names you think best.  The use of numerical suffixes were an attempt to maintain alignment within AArch64SVEInstrInfo, but there's no requirement for that.  I will say the names are somewhat irrelevant though, it's the instruction names that people will be searching for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137411