[PATCH] D129282: [AArch64][SVE] Ensure PTEST operands have type nxv16i1

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16178
          "Expected legal scalable vector type!");
 
   // Ensure target specific opcodes are using legal type.
----------------
Does adding an assert that `Op->getValueType() == Pg->getValueTypes()` cause any issues, because otherwise I think the `AArch64CC::ANY_ACTIVE` code in unsafe?


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:782-783
+  def PTEST_PP : sve_int_ptest<0b010000, "ptest">,
+                 Pat<(AArch64ptest (nxv16i1 PPR:$pg), (nxv16i1 PPR:$src)),
+                     (PTEST_PP PPR:$pg, PPR:$src)>;
   defm PFALSE  : sve_int_pfalse<0b000000, "pfalse">;
----------------
sdesmalen wrote:
> nit: We normally put the pattern in the class definition itself, which has most benefits if there are multiple instructions being instantiated from the same (multi)class, so that we only have to specify the pattern once. In this case, there's only one instantiation from `sve_int_ptest`, but for consistency and to keep this file tidy, I'd rather see the pattern living in that class.
> 
> This means you'll need to make `sve_int_ptest` a multiclass and pass in `AArch64ptest` as a `SDPatternOperator`. You can then also use `SVE_2_Op_Pat` for the pattern itself.
> See for example the definition of `class sve_int_pfirst`.
You can also do this within `sve_int_ptest` directly by just passing in `AArch64ptest`. See `sve_int_wrffr` for inspiration.


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

https://reviews.llvm.org/D129282