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

[Allen zhong via Phabricator via llvm-commits]

Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:495-499
+  case AArch64::DestructiveBinary:
+    // Don't check the SrcIdx for DOPRegIsUnique to avoid the crash, will be
+    // addressed by additional LSL when necessary.
+    DOPRegIsUnique = DstReg == MI.getOperand(DOPIdx).getReg();
+    break;
----------------
paulwalker-arm wrote:
> Rather than forcing `DOPRegIsUnique` to an incorrect value perhaps this switch serves a different purpose and at least one[1] of the matching asserts are just not relevant anymore.
> 
> Your fix is essentially saying "if we cannot prefix the requested instruction we'll instead emit a prefixed_zeroing_mov".  Which suggests this fixes the problem for all `DType`s and thus we no longer require `DOPRegIsUnique == true` for correctness (although it is a hint to slightly poor code generation).
> 
> [1] I say at least one because for this patch you only care about the zering case so really the `Create the additional LSL to zero` code belongs in the `if (FalseZero)` block and thus the other `DOPRegIsUnique` assert remains valid.  That said we can always emit a normal COPY/MOV for the `!FalseZero` case which it one instruction rather than two. However, that's not a scenario that can occur with the current code so you wouldn't be able to write tests and thus best avoided.
Done, delete the code about forcing DOPRegIsUnique to an incorrect value, thanks.

This is version is expect to fix the DestructiveBinary only to begin with.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:591-592
+  // 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:
> If you keep the above code then does `if (!DOPRegIsUnique)` work here? and as mentioned I believe it should sit with the `if (FalseZero)` block.
* No, the above code is only active when the **#ifndef NDEBUG** is true, so it is depend on our configue.

* Apply your comment and move into the **if (FalseZero)** block, thanks


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

https://reviews.llvm.org/D124325