[PATCH] D124325: [AArch64][SVE] Support logical operation BIC with DestructiveBinary patterns

[Allen zhong via Phabricator via llvm-commits]

Allen marked an inline comment as done.
Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:454-455
-
-  if (DType == AArch64::DestructiveBinary)
-    assert(DstReg != MI.getOperand(3).getReg());
-
----------------
paulwalker-arm wrote:
> Is it possible to move this logic into the `DOPRegIsUnique` switch statement like
> ```
> case AArch64::DestructiveBinary:
>   DOPRegIsUnique = DstReg != MI.getOperand(SrcIdx).getReg();
> ```
> 
Apply your comment, thanks


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:558
+    // prefixed_zeroing_mov for DestructiveBinary.
+    assert((AArch64::DestructiveBinary == DType || DOPRegIsUnique) &&
+           "The destructive operand should be unique");
----------------
paulwalker-arm wrote:
> I think this reads better as `DOPRegIsUnique || AArch64::DestructiveBinary == DType` because the second part os the exception.
Done


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:575-576
+    // movprfx z0.b, p0/z, z0.b; lsl z0.b, p0/m, z0.b, #0;
+    if (DType == AArch64::DestructiveBinary &&
+        DstReg == MI.getOperand(SrcIdx).getReg()) {
+      BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(LSLZero))
----------------
paulwalker-arm wrote:
> With my previous suggestion does
> ```
> if (DType == AArch64::DestructiveBinary && !DOPRegIsUnique)
> ```
> work here?
> 
Yes, it works with your previous suggestion 'DOPRegIsUnique = DstReg != MI.getOperand(SrcIdx).getReg();'


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

https://reviews.llvm.org/D124325