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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm 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;
----------------
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.


================
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))
----------------
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.


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

https://reviews.llvm.org/D124325