[PATCH] D74856: [AArch64][SVE] Add backend support for splats of immediates

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:13
 
+def SVE8BitLslImm : ComplexPattern<i32, 2, "SelectSVE8BitLslImm", [imm]>;
+
----------------
cameron.mcinally wrote:
> efriedma wrote:
> > cameron.mcinally wrote:
> > > cameron.mcinally wrote:
> > > > efriedma wrote:
> > > > > cameron.mcinally wrote:
> > > > > > efriedma wrote:
> > > > > > > Is there some reason you can't use the existing cpy_imm8_opt_lsl_i8?
> > > > > > I'm not sure I understand this one. What should I replace with cpy_imm8_opt_lsl_i8?
> > > > > > 
> > > > > > SVE8BitLslImm is looking for two i8 immediates (i8 value and i8 shift amount).
> > > > > > 
> > > > > > cpy_imm8_opt_lsl_i8 is just checking for one i8 immediate, IINM.
> > > > > "class imm8_opt_lsl" has some code which looks like it supposed to be used for matching.  Granted, it's using ImmLeaf, so maybe it's just broken.
> > > > Oh, yeah, I see what you mean now. I'll dig into that...
> > > Looking closer, this seems to be in line with the other ComplexPattern uses (e.g. SVEAddSubImm8Pat).
> > > 
> > > The SVE8BitLslImm ComplexPattern is used to match the two i8 operands, $imm and $shift. If I tried to use cpy_imm8_opt_lsl_i64 in its place, I don't see a way to get the individual operands by name. E.g. something like:
> > > 
> > > ```
> > >   def : Pat<(nxv2i64 (AArch64dup (i64 (cpy_imm8_opt_lsl_i64 i64:$imm)))),
> > >             (DUP_ZI_D $???, $???)>;
> > > ```
> > If you write something like `SVE8BitLslImm:$a`, it actually counts as two operands, I think, because that's what the ComplexPattern specifies?  Not completely sure.
> > 
> > Orthogonal to that, if we aren't going to use the predicates in cpy_imm8_opt_lsl_i64 etc., we should get rid of them.
> > If you write something like SVE8BitLslImm:$a, it actually counts as two operands, I think, because that's what the ComplexPattern specifies? Not completely sure.
> 
> `SVE8BitLslImm` usage would be something like this:
> 
> `(SVE8BitLslImm i32:$a, i32:$b)`
> 
> where `a` is an immediate value and `b` is a shift amount. I think that's necessary, having two separate named immediates, since `DUP_ZI_x` expects two immediate operands. E.g.
> 
> ```
>   def : Pat<(nxv2i64 (AArch64dup (i64 (SVE8BitLslImm i32:$a, i32:$b)))),
>             (DUP_ZI_D $a, $b)>;
> ```
> 
> > Orthogonal to that, if we aren't going to use the predicates in cpy_imm8_opt_lsl_i64 etc., we should get rid of them.
> 
> We do use `cpy_imm8_opt_lsl_i64` in `DUP_ZI_x` right now, but it's slightly different:
> 
> ```
>   def : InstAlias<"mov $Zd, $imm",
>                   (!cast<Instruction>(NAME # _D) ZPR64:$Zd, cpy_imm8_opt_lsl_i64:$imm), 1>;
> ```
> 
> `cpy_imm8_opt_lsl_i64` will match to `(ops i32imm, i32imm)`, which are the two operands of the `DUP_ZI_x`. This seems desirable since the hardware wants a single immediate constructed from the 2 immediate parts. That is, we want a way to verify that the two immediates actually make sense together.
> 
> All this is really at the edge of my TblGen understanding, so take with some skepticism...
Looking a little more, I think I'm just wrong on the ComplexPattern thing.

> We do use cpy_imm8_opt_lsl_i64 in DUP_ZI_x right now, but it's slightly different:

That's not using the IntImm predicate; there's a separate asm operand parser function.


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

https://reviews.llvm.org/D74856