[PATCH] D124224: [AArch64][SVE] Add some logical operation DestructiveBinaryComm patterns

[Allen zhong via Phabricator via llvm-commits]

Allen added a comment.

In D124224#3467729 <https://reviews.llvm.org/D124224#3467729>, @thakis wrote:

> Thanks!

@thakis sorry for this patch
@paulwalker-arm thanks very much for helping fix this issue



================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:342
   defm SUBR_ZPmZ : sve_int_bin_pred_arit_0<0b011, "subr", "SUBR_ZPZZ", int_aarch64_sve_subr, DestructiveBinaryCommWithRev, "SUB_ZPmZ", /*isReverseInstr*/ 1>;
 } // End HasSVEorStreamingSVE
 
----------------
paulwalker-arm wrote:
> If you don't mind indulging my OCD I've tried to ensure the pseudo instructions are defined after the real ones so can you move the definitions for `ORR_ZPmZ`..`BIC_ZPmZ` to the bottom of this block so they're before the following `UseExperimentalZeroingPseudos` block.
Apply your review, thanks


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:359
+  defm AND_ZPmZ : sve_int_bin_pred_log<0b010, "and", "AND_ZPZZ", int_aarch64_sve_and, DestructiveBinaryComm>;
+  defm BIC_ZPmZ : sve_int_bin_pred_log<0b011, "bic", "BIC_ZPZZ", int_aarch64_sve_bic, DestructiveBinaryComm>;
 
----------------
paulwalker-arm wrote:
> `BIC` (i.e. `A & ~B`) is not a commutative operation and so should use `DestructiveBinary`.
good catch, thanks for your carefully review


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:2776
+  def _B : sve_int_bin_pred_arit_log<0b00, 0b11, opc, asm, ZPR8>,
+             SVEPseudo2Instr<Ps # _B, 1>, SVEInstr2Rev<NAME # _B, revname # _B, isOrig>;
+  def _H : sve_int_bin_pred_arit_log<0b01, 0b11, opc, asm, ZPR16>,
----------------
david-arm wrote:
> Allen wrote:
> > david-arm wrote:
> > > Again, I don't think we need any of these for the logical operations.
> > Thanks very much , can you help to detailly point out  what's "these" ? I may don't get your idea?
> Sorry I wasn't being very clear - I was relying upon my comment highlighting the right part of the code! I just mean that the ", SVEInstr2Rev<...>" bits are unnecessary I think? We don't have any reverse logical operations so perhaps we can just delete it? If so, you can also just delete the 'revname' and 'isOrig' arguments passed to the multiclass as well.
Thanks very much for explaining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124224