[PATCH] D76901: [AArch64][SVE] Add support for boolean logic and fcmp.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7563
+      if (ConstVal->isOne())
+        return getPTrue(DAG, dl, VT, AArch64SVEPredPattern::all);
+      // TODO: Add special case for constant false
----------------
efriedma wrote:
> sdesmalen wrote:
> > It seems like this code is missing a guard for `VT.isScalableVector()` (same for the `aarch64_sve_whilelo` case below)
> Not sure what you think needs to be handled; non-scalable i1 vectors aren't legal.
Yes, of course! Thanks for adding that comment.


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1369
   def : SVE_3_Op_Pat<nxv2i1, op, nxv2i1, nxv2i1, nxv2i1, !cast<Instruction>(NAME)>;
+  def : Pat<(nxv2i1 (op_nopred (nxv2i1 PPRAny:$Pn), (nxv2i1 PPRAny:$Pm))),
+            (!cast<Instruction>(NAME) (PTRUE_D 31), PPRAny:$Pn, PPRAny:$Pm)>;
----------------
efriedma wrote:
> cameron.mcinally wrote:
> > efriedma wrote:
> > > sdesmalen wrote:
> > > > Do we want to create a `SVE_2_Op_<..>_Pat` or something for these? There will be other instructions where this would be useful as well.
> > > > 
> > > > nit: please reverse the order so it matches the patterns straight above it.
> > > I'm not sure what that would look like.  Are you proposing something like SVE_2_Op_PTRUE_H_Pat/SVE_2_Op_PTRUE_S_Pat/etc.?
> > There's something similar in D71712:
> > 
> > ```
> > class SVE_2_Op_AllActive_Pat<ValueType vtd, SDPatternOperator op, ValueType vt1,
> >                              ValueType vt2, Instruction inst, Instruction ptrue>
> > : Pat<(vtd (op vt1:$Op1, vt2:$Op2)),
> >       (inst (ptrue 31), $Op1, $Op2)>;
> > ```
> > 
> > Seems like a good fit, but I'm not sure...
> So this would be written like the following?
> 
> ```
>   def : SVE_2_Op_AllActive_Pat<nxv2i1, op_nopred, nxv2i1, nxv2i1,
>                                !cast<Instruction>(NAME), PTRUE_D>;
> ```
> 
> It doesn't seem much better than what I've written, but okay, I guess.
Cheers, FWIW this makes it a bit more readable to me (especially in conjunction with the use of SVE_3_Op_Pat above).


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:1376
+                               !cast<Instruction>(NAME), PTRUE_B>;
+  def : SVE_2_Op_AllActive_Pat<nxv8i1, op_nopred, nxv8i1, nxv8i1,
+                               !cast<Instruction>(NAME), PTRUE_H>;
----------------
This file doesn't seem too strict on the 80char limit, so I think having this on the same line would make it more readable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76901