[AArch64][SVE] Support logical operation BIC with DestructiveBinary patterns

[Allen]

Summary

Logical operation BIC with DestructiveBinary patterns is temporarily removed as
causes an assert (commit 3c382ed71f15), so try to fix that.
The most significant being that for pseudo instructions that do not have real instructions (including movpfx'd ones) that cover all combinations of register allocation, their expansion will be broken. This is the main reason the zeroing is an experimental feature because it has known bugs.
So we add an extra LSL for movprfx expand BIC_ZPZZ_ZERO A, P, A, A when necessary.

movprfx	z0.s, p0/z, z0.s
lsl z0.b, p0/m, z0.b, #0
bic	z0.s, p0/m, z0.s, z0.s

Depends on D88595

[Allen]

add a new test case

[paulwalker-arm]

Hi @Allen, you might be interested in D88595.

This is the main reason the zeroing is an experimental feature because it has known bugs. The most significant being that for pseudo instructions that do not have real instructions (including movpfx'd ones) that cover all combinations of register allocation, their expansion will be broken. I believe BIC is an example of this because there's no way to movprfx expand BIC_ZPZZ_ZERO A, P, A, A. An unrealistic usage I know but it's still legitamet.

define <vscale x 4 x i32> @foo(<vscale x 4 x i1> %pg, <vscale x 4 x i32> %a) #0 {
entry:
  %t1 = select <vscale x 4 x i1> %pg, <vscale x 4 x i32> %a, <vscale x 4 x i32> zeroinitializer
  %t2 = tail call <vscale x 4 x i32> @llvm.aarch64.sve.bic.nxv4i32(<vscale x 4 x i1> %pg, <vscale x 4 x i32> %t1, <vscale x 4 x i32> %a)
  ret <vscale x 4 x i32> %t2
}

This will trigger an assert, even after this patch, because otherwise it would emit

movprfx	z0.s, p0/z, z0.s
bic	z0.s, p0/m, z0.s, z0.s

which is not a valid use of movprfx.

For ACfL(https://developer.arm.com/tools-and-software/server-and-hpc/compile/arm-compiler-for-linux) we solve this using a couple of machine function passes but I feel they're not the correct solution hence the above patch where we'd rather have a mechanism to better express the register requirements as part of the pseudo's definition.

[Allen]

In D124325#3469815, @paulwalker-arm wrote:

Hi @Allen, you might be interested in D88595.

This is the main reason the zeroing is an experimental feature because it has known bugs. The most significant being that for pseudo instructions that do not have real instructions (including movpfx'd ones) that cover all combinations of register allocation, their expansion will be broken. I believe BIC is an example of this because there's no way to movprfx expand BIC_ZPZZ_ZERO A, P, A, A. An unrealistic usage I know but it's still legitamet.

define <vscale x 4 x i32> @foo(<vscale x 4 x i1> %pg, <vscale x 4 x i32> %a) #0 {
entry:
  %t1 = select <vscale x 4 x i1> %pg, <vscale x 4 x i32> %a, <vscale x 4 x i32> zeroinitializer
  %t2 = tail call <vscale x 4 x i32> @llvm.aarch64.sve.bic.nxv4i32(<vscale x 4 x i1> %pg, <vscale x 4 x i32> %t1, <vscale x 4 x i32> %a)
  ret <vscale x 4 x i32> %t2
}

This will trigger an assert, even after this patch, because otherwise it would emit

movprfx	z0.s, p0/z, z0.s
bic	z0.s, p0/m, z0.s, z0.s

which is not a valid use of movprfx.

For ACfL(https://developer.arm.com/tools-and-software/server-and-hpc/compile/arm-compiler-for-linux) we solve this using a couple of machine function passes but I feel they're not the correct solution hence the above patch where we'd rather have a mechanism to better express the register requirements as part of the pseudo's definition.

Thanks @paulwalker-arm for detail explaning the issue of of register allocation. After read the D88595, I think there is two candidate ways to address this, do I missing something ？

1、the instruction bic is special, so when all the registers are same, it should be expansion with zero  (i.e. BIC (A, A) = A & ~A = 0)

2、make use of  not_all_same, and  {not_all_same(Dst, Op0, Op1), earlyclobber(Dst)} can be simplified as {earlyclobber(Dst)}, which can guard the registers without all same.

[paulwalker-arm]

I don't think either of these are good options. I don't like the idea of having instruction specific handling within the expand code and I'm not sure where the logic from (2) comes from but regardless using earlyclobber in this way will force Dst to be unique and thus forces the need for movpfrx (i.e. it blocks the BIC_ZPZZ_ZERO A, P, A, B use case ).

To my mind the best option is to allow the register requirements to be better expressed as part of the pseudo instruction. Until then I cannot see how we can move this feature out of its experimental phase. Perhaps there's an alternative to D88595 but I've not thought about it for a while. How important is movpfrx based zeroing to you right now? Have you got a problematic use case you can share?

[Allen]

In D124325#3475793, @paulwalker-arm wrote:

I don't think either of these are good options. I don't like the idea of having instruction specific handling within the expand code and I'm not sure where the logic from (2) comes from but regardless using earlyclobber in this way will force Dst to be unique and thus forces the need for movpfrx (i.e. it blocks the BIC_ZPZZ_ZERO A, P, A, B use case ).

To my mind the best option is to allow the register requirements to be better expressed as part of the pseudo instruction. Until then I cannot see how we can move this feature out of its experimental phase. Perhaps there's an alternative to D88595 but I've not thought about it for a while. How important is movpfrx based zeroing to you right now? Have you got a problematic use case you can share?

Thanks, I'll wait for D88595 , and I'm not in a hurry for this, but only confirm the idea.
BTW, the logic from (2) comes from the comment https://reviews.llvm.org/D88595#inline-886782

[efriedma]

I don't like the idea of having instruction specific handling within the expand code

Would this really be so terrible? I mean, it's arguably a bit of a hack, but it's not that different from the way we handle other pseudo-instructions.

[paulwalker-arm]

In D124325#3481275, @efriedma wrote:

I don't like the idea of having instruction specific handling within the expand code

Would this really be so terrible? I mean, it's arguably a bit of a hack, but it's not that different from the way we handle other pseudo-instructions.

I think so. Pseudo-instruction expansion is often instruction specific but for the movprfx handling we've detached the logic from the instructions (because there's 100s of them) and instead split them across various categories. So I'd much rather see problems solved for a whole category rather than partially for a single instruction within a category.

[efriedma]

In D124325#3484543, @paulwalker-arm wrote:

In D124325#3481275, @efriedma wrote:

I don't like the idea of having instruction specific handling within the expand code

Would this really be so terrible? I mean, it's arguably a bit of a hack, but it's not that different from the way we handle other pseudo-instructions.

I think so. Pseudo-instruction expansion is often instruction specific but for the movprfx handling we've detached the logic from the instructions (because there's 100s of them) and instead split them across various categories. So I'd much rather see problems solved for a whole category rather than partially for a single instruction within a category.

I see what you mean. And I guess some of the instructions don't have any sort of "identity" result like this.

I guess you could use an alternative sequence. I can't come up with a two-instruction sequence, but I guess you can movprfx a dummy instruction, like movprfx z0.b, p0/z, z0.b; add z0.b, z0.b, #0; bic z0.b, p0/m, z0.b, z0.b.

[Allen]

In D124325#3485956, @efriedma wrote:

In D124325#3484543, @paulwalker-arm wrote:

In D124325#3481275, @efriedma wrote:

I don't like the idea of having instruction specific handling within the expand code

Would this really be so terrible? I mean, it's arguably a bit of a hack, but it's not that different from the way we handle other pseudo-instructions.

I think so. Pseudo-instruction expansion is often instruction specific but for the movprfx handling we've detached the logic from the instructions (because there's 100s of them) and instead split them across various categories. So I'd much rather see problems solved for a whole category rather than partially for a single instruction within a category.

I see what you mean. And I guess some of the instructions don't have any sort of "identity" result like this.

I guess you could use an alternative sequence. I can't come up with a two-instruction sequence, but I guess you can movprfx a dummy instruction, like movprfx z0.b, p0/z, z0.b; add z0.b, z0.b, #0; bic z0.b, p0/m, z0.b, z0.b.

As D88595 will not be accepted in a short time, so I'll try with your idea.
Before doing so, I want to check your suggestion:
do you mean the additonal add z0.b, z0.b, #0; should match the above movprfx instuction(meet the constraints of movprfx instruction), then the bic instruction will get "identity" result?

[efriedma]

The sequence movprfx z0.b, p0/z, z0.b; add z0.b, z0.b, #0; zeros the lanes in z0 that aren't active in p0. (It's a slightly weird way to write it, but as far as I know there isn't any single instruction with equivalent semantics.) Then we can just use the regular instruction that leaves the lanes we just zeroed unmodified. This works regardless of the operation (as long as it doesn't do cross-lane processing).

The "identity" thing is just noting that if the two operands to bic are identical, the result is always zero. But as noted before, that doesn't generalize to other operations.

[Allen]

Add additional add z0.b, z0.b, #0 according comment

[efriedma]

I think you're missing the point... we only want to insert the extra add instruction if the movprfx would be otherwise be illegal.

[Allen]

Add additional add z0.b, z0.b, #0 when necessary

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

593

I don't believe this is safe because only predicated instructions are allowed to follow a predicated movprfx instruction. There's a section within

https://developer.arm.com/documentation/ddi0487/latest/

Data processing - SVE
-> Move operations
--> Move prefix

that details which instructions are allowed to follow a movprfx.

593–601

Do any of the tests exercise this code?

[efriedma]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

593

Oh, oops, I misread the documentation. Can we use lsl z0.b, p0/m, z0.b, #0 instead?

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

593

I think so but will check if there's an architecture preferred answer and will report back.

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

593

Thanks @paulwalker-arm and @efriedma.
If we don't have a better architecture preferred answer, how about using lsl z0.b, p0/m, z0.b, #0 first? In fact, there are very few scenarios where this additional instruction is required, such as case bic_i64_zero_no_comm.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

593

lsl is not a great fit for current implementations but as you say this will only be used in rare cases that we ultimately want to solve during register allocation anyway. Plus, looking over the instruction set I'm not sure there's a non-shift alternative so yes let's go with lsl z0.b, p0/m, z0.b, #0.

[Allen]

Update add z0.b, z0.b, #0 with lsl z0.b, p0/m, z0.b, #0 as comment

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

593

Done, thanks for your suggestion.

[Allen]

ping ?

[efriedma]

Still missing a testcase that actually triggers the "lsl" codepath.

[Allen]

Add a mir case to trigger the "lsl" codepath.

[Allen]

any new suggestion about the last update? Thanks.

[Allen]

ping ?

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

498–502

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 DTypes 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.

594–595

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.

[Allen]

address comments

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

498–502

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.

594–595

• 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

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

594–595

Sure, but what I'm suggesting is that the code is no longer DEBUG only as we now have a real world use for it.

[paulwalker-arm]

@Allen It looks like something has gone wrong because the latest version looks like a different piece of work, unrelated to movprfx.

[Allen]

update to fix the different piece of work

[Allen]

In D124325#3902259, @paulwalker-arm wrote:

@Allen It looks like something has gone wrong because the latest version looks like a different piece of work, unrelated to movprfx.

Oh. sorry for the mistake, and now it is the right version

[Allen]

ping ?

[paulwalker-arm]

@Allen Did you see my comment from "Wed, Nov 2, 2:34 PM"? Not a requirement but I figure moving this code out of DEBUG and ensuring DOPRegIsUnique is correct for AArch64::DestructiveBinary might be better in the long run. I know at this time you only care about DestructiveBinary, but I think your solution can easily be extended to the other destructive types later on, although part of me thinks they'll just work after this patch.

[Allen]

Delete the NDEBUG as comment, thanks

[Allen]

In D124325#3911789, @paulwalker-arm wrote:

@Allen Did you see my comment from "Wed, Nov 2, 2:34 PM"? Not a requirement but I figure moving this code out of DEBUG and ensuring DOPRegIsUnique is correct for AArch64::DestructiveBinary might be better in the long run. I know at this time you only care about DestructiveBinary, but I think your solution can easily be extended to the other destructive types later on, although part of me thinks they'll just work after this patch.

sorry @paulwalker-arm for missing that. Now I deleted the DEBUG.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

454–455

Is it possible to move this logic into the DOPRegIsUnique switch statement like

case AArch64::DestructiveBinary:
  DOPRegIsUnique = DstReg != MI.getOperand(SrcIdx).getReg();

571

I think this reads better as DOPRegIsUnique || AArch64::DestructiveBinary == DType because the second part os the exception.

585–586

With my previous suggestion does

if (DType == AArch64::DestructiveBinary && !DOPRegIsUnique)

work here?

[Allen]

Address comment

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

454–455

Apply your comment, thanks

571

Done

585–586

Yes, it works with your previous suggestion 'DOPRegIsUnique = DstReg != MI.getOperand(SrcIdx).getReg();'

[Allen]

closed with commit ffb109b6852d248c9d2e3202477dccf20aac7151

[rupprecht]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

499

Is fall through intended here? I assume you want a break;?

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:495:3: warning: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
  case AArch64::DestructiveBinaryComm:

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

499

Yes, I'll add break, thanks

[rupprecht]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

499

Added a break in 094c0eccdf959c3b9c85219e33c3fcfbab024b61 to avoid fallthrough. Please take a look if that's not what you want.

[rupprecht]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

499

Ah, race condition -- I applied the break at the same time you responded. Glad I assumed correctly :)

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

499

Thanks for fixing.

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

bool AArch64ExpandPseudo::expand_DestructiveOp(
   unsigned Opcode = AArch64::getSVEPseudoMap(MI.getOpcode());
   uint64_t DType = TII->get(Opcode).TSFlags & AArch64::DestructiveInstTypeMask;
   uint64_t FalseLanes = MI.getDesc().TSFlags & AArch64::FalseLanesMask;
   bool FalseZero = FalseLanes == AArch64::FalseLanesZero;
 
   Register DstReg = MI.getOperand(0).getReg();
   bool DstIsDead = MI.getOperand(0).isDead();
 
   if (DType == AArch64::DestructiveBinary)
     assert(DstReg != MI.getOperand(3).getReg());
 
   bool UseRev = false;
   unsigned PredIdx, DOPIdx, SrcIdx, Src2Idx;
   switch (DType) {
   case AArch64::DestructiveBinaryComm:
   case AArch64::DestructiveBinaryCommWithRev:
     if (DstReg == MI.getOperand(3).getReg()) {
       // FSUB Zd, Pg, Zs1, Zd  ==> FSUBR   Zd, Pg/m, Zd, Zs1
bool AArch64ExpandPseudo::expand_DestructiveOp(
   }
 
 #ifndef NDEBUG
   // MOVPRFX can only be used if the destination operand
   // is the destructive operand, not as any other operand,
   // so the Destructive Operand must be unique.
   bool DOPRegIsUnique = false;
   switch (DType) {
+  case AArch64::DestructiveBinary:
+    // Zeros the lanes in z0 that aren't active in p0 with sequence movprfx
+    // z0.b, p0/z, z0.b; add z0.b, z0.b, #0;
+    DOPRegIsUnique = true;
+    break;
   case AArch64::DestructiveBinaryComm:
   case AArch64::DestructiveBinaryCommWithRev:
     DOPRegIsUnique =
       DstReg != MI.getOperand(DOPIdx).getReg() ||
       MI.getOperand(DOPIdx).getReg() != MI.getOperand(SrcIdx).getReg();
     break;
   case AArch64::DestructiveUnaryPassthru:
   case AArch64::DestructiveBinaryImm:
if ((NewOpcode = AArch64::getSVERevInstr(Opcode)) != -1)
       Opcode = NewOpcode;
     // e.g. DIVR -> DIV
     else if ((NewOpcode = AArch64::getSVENonRevInstr(Opcode)) != -1)
       Opcode = NewOpcode;
   }
 
   // Get the right MOVPRFX
   uint64_t ElementSize = TII->getElementSizeForOpcode(Opcode);
-  unsigned MovPrfx, MovPrfxZero;
+  unsigned MovPrfx, AddZero, MovPrfxZero;
   switch (ElementSize) {
   case AArch64::ElementSizeNone:
   case AArch64::ElementSizeB:
     MovPrfx = AArch64::MOVPRFX_ZZ;
+    AddZero = AArch64::ADD_ZI_B;
     MovPrfxZero = AArch64::MOVPRFX_ZPzZ_B;
     break;
   case AArch64::ElementSizeH:
     MovPrfx = AArch64::MOVPRFX_ZZ;
+    AddZero = AArch64::ADD_ZI_H;
     MovPrfxZero = AArch64::MOVPRFX_ZPzZ_H;
     break;
   case AArch64::ElementSizeS:
     MovPrfx = AArch64::MOVPRFX_ZZ;
+    AddZero = AArch64::ADD_ZI_S;
     MovPrfxZero = AArch64::MOVPRFX_ZPzZ_S;
     break;
   case AArch64::ElementSizeD:
     MovPrfx = AArch64::MOVPRFX_ZZ;
+    AddZero = AArch64::ADD_ZI_D;
     MovPrfxZero = AArch64::MOVPRFX_ZPzZ_D;
     break;
   default:
     llvm_unreachable("Unsupported ElementSize");
   }
 
   //
   // Create the destructive operation (if required)
   //
-  MachineInstrBuilder PRFX, DOP;
+  MachineInstrBuilder PRFX, DOP, ADD;
   if (FalseZero) {
 #ifndef NDEBUG
     assert(DOPRegIsUnique && "The destructive operand should be unique");
 #endif
     assert(ElementSize != AArch64::ElementSizeNone &&
            "This instruction is unpredicated");
 
     // Merge source operand into destination register
     PRFX = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(MovPrfxZero))
                .addReg(DstReg, RegState::Define)
                .addReg(MI.getOperand(PredIdx).getReg())
                .addReg(MI.getOperand(DOPIdx).getReg());
 
     // After the movprfx, the destructive operand is same as Dst
     DOPIdx = 0;
   } else if (DstReg != MI.getOperand(DOPIdx).getReg()) {
 #ifndef NDEBUG
     assert(DOPRegIsUnique && "The destructive operand should be unique");
 #endif
     PRFX = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(MovPrfx))
                .addReg(DstReg, RegState::Define)
                .addReg(MI.getOperand(DOPIdx).getReg());
     DOPIdx = 0;
   }
 
+  // Create the additional ADD to zero the lanes
+  if (DType == AArch64::DestructiveBinary)
+    ADD = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(AddZero))
+              .addReg(DstReg, RegState::Define)
+              .addReg(DstReg)
+              .addImm(0)
+              .addImm(0);
+
   //
   // Create the destructive operation
   //
   DOP = BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(Opcode))
     .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead));
 
   switch (DType) {
   case AArch64::DestructiveUnaryPassthru:
     DOP.addReg(MI.getOperand(DOPIdx).getReg(), RegState::Kill)
         .add(MI.getOperand(PredIdx))
         .add(MI.getOperand(SrcIdx));
     break;
+  case AArch64::DestructiveBinary:
   case AArch64::DestructiveBinaryImm:
   case AArch64::DestructiveBinaryComm:
   case AArch64::DestructiveBinaryCommWithRev:
     DOP.add(MI.getOperand(PredIdx))
        .addReg(MI.getOperand(DOPIdx).getReg(), RegState::Kill)
        .add(MI.getOperand(SrcIdx));
     break;
   case AArch64::DestructiveTernaryCommWithRev:

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td

 let Predicates = [HasSVEorSME, UseExperimentalZeroingPseudos] in {
   defm ADD_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_add>;
   defm SUB_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_sub>;
   defm SUBR_ZPZZ : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_subr>;
 
   defm ORR_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_orr>;
   defm EOR_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_eor>;
   defm AND_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_and>;
-  defm BIC_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<null_frag>;
+  defm BIC_ZPZZ  : sve_int_bin_pred_zeroing_bhsd<int_aarch64_sve_bic>;
 } // End HasSVEorSME, UseExperimentalZeroingPseudos
 
 let Predicates = [HasSVEorSME] in {
   defm ADD_ZI   : sve_int_arith_imm0<0b000, "add", add>;
   defm SUB_ZI   : sve_int_arith_imm0<0b001, "sub", sub>;
   defm SUBR_ZI  : sve_int_arith_imm0<0b011, "subr", AArch64subr>;
   defm SQADD_ZI : sve_int_arith_imm0<0b100, "sqadd", saddsat>;
   defm UQADD_ZI : sve_int_arith_imm0<0b101, "uqadd", uaddsat>;

llvm/test/CodeGen/AArch64/sve-intrinsics-int-arith-merging.ll

 
 ;
 ; BIC
 ;
 
 define <vscale x 16 x i8> @bic_i8_zero(<vscale x 16 x i1> %pg, <vscale x 16 x i8> %a, <vscale x 16 x i8> %b) {
 ; CHECK-LABEL: bic_i8_zero:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov z2.b, #0 // =0x0
-; CHECK-NEXT:    sel z0.b, p0, z0.b, z2.b
+; CHECK-NEXT:    movprfx z0.b, p0/z, z0.b
+; CHECK-NEXT:    add z0.b, z0.b, #0 // =0x0
 ; CHECK-NEXT:    bic z0.b, p0/m, z0.b, z1.b
 ; CHECK-NEXT:    ret
   %a_z = select <vscale x 16 x i1> %pg, <vscale x 16 x i8> %a, <vscale x 16 x i8> zeroinitializer
   %out = call <vscale x 16 x i8> @llvm.aarch64.sve.bic.nxv16i8(<vscale x 16 x i1> %pg,
                                                                <vscale x 16 x i8> %a_z,
                                                                <vscale x 16 x i8> %b)
   ret <vscale x 16 x i8> %out
 }
 
 define <vscale x 8 x i16> @bic_i16_zero(<vscale x 8 x i1> %pg, <vscale x 8 x i16> %a, <vscale x 8 x i16> %b) {
 ; CHECK-LABEL: bic_i16_zero:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov z2.h, #0 // =0x0
-; CHECK-NEXT:    sel z0.h, p0, z0.h, z2.h
+; CHECK-NEXT:    movprfx z0.h, p0/z, z0.h
+; CHECK-NEXT:    add z0.h, z0.h, #0 // =0x0
 ; CHECK-NEXT:    bic z0.h, p0/m, z0.h, z1.h
 ; CHECK-NEXT:    ret
   %a_z = select <vscale x 8 x i1> %pg, <vscale x 8 x i16> %a, <vscale x 8 x i16> zeroinitializer
   %out = call <vscale x 8 x i16> @llvm.aarch64.sve.bic.nxv8i16(<vscale x 8 x i1> %pg,
                                                                <vscale x 8 x i16> %a_z,
                                                                <vscale x 8 x i16> %b)
   ret <vscale x 8 x i16> %out
 }
 
 define <vscale x 4 x i32> @bic_i32_zero(<vscale x 4 x i1> %pg, <vscale x 4 x i32> %a, <vscale x 4 x i32> %b) {
 ; CHECK-LABEL: bic_i32_zero:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov z2.s, #0 // =0x0
-; CHECK-NEXT:    sel z0.s, p0, z0.s, z2.s
+; CHECK-NEXT:    movprfx z0.s, p0/z, z0.s
+; CHECK-NEXT:    add z0.s, z0.s, #0 // =0x0
 ; CHECK-NEXT:    bic z0.s, p0/m, z0.s, z1.s
 ; CHECK-NEXT:    ret
   %a_z = select <vscale x 4 x i1> %pg, <vscale x 4 x i32> %a, <vscale x 4 x i32> zeroinitializer
   %out = call <vscale x 4 x i32> @llvm.aarch64.sve.bic.nxv4i32(<vscale x 4 x i1> %pg,
                                                                <vscale x 4 x i32> %a_z,
                                                                <vscale x 4 x i32> %b)
   ret <vscale x 4 x i32> %out
 }
 
 define <vscale x 2 x i64> @bic_i64_zero(<vscale x 2 x i1> %pg, <vscale x 2 x i64> %a, <vscale x 2 x i64> %b) {
 ; CHECK-LABEL: bic_i64_zero:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov z2.d, #0 // =0x0
-; CHECK-NEXT:    sel z0.d, p0, z0.d, z2.d
+; CHECK-NEXT:    movprfx z0.d, p0/z, z0.d
+; CHECK-NEXT:    add z0.d, z0.d, #0 // =0x0
 ; CHECK-NEXT:    bic z0.d, p0/m, z0.d, z1.d
 ; CHECK-NEXT:    ret
   %a_z = select <vscale x 2 x i1> %pg, <vscale x 2 x i64> %a, <vscale x 2 x i64> zeroinitializer
   %out = call <vscale x 2 x i64> @llvm.aarch64.sve.bic.nxv2i64(<vscale x 2 x i1> %pg,
                                                                <vscale x 2 x i64> %a_z,
                                                                <vscale x 2 x i64> %b)
   ret <vscale x 2 x i64> %out
 }
 
+; BIC (i.e. A & ~B) is not a commutative operation, so disable it when the
+; destination operand is not the destructive operand
+define <vscale x 2 x i64> @bic_i64_zero_no_comm(<vscale x 2 x i1> %pg, <vscale x 2 x i64> %a, <vscale x 2 x i64> %b) {
+; CHECK-LABEL: bic_i64_zero_no_comm:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    mov z2.d, #0 // =0x0
+; CHECK-NEXT:    sel z0.d, p0, z0.d, z2.d
+; CHECK-NEXT:    bic z1.d, p0/m, z1.d, z0.d
+; CHECK-NEXT:    mov z0.d, z1.d
+; CHECK-NEXT:    ret
+  %a_z = select <vscale x 2 x i1> %pg, <vscale x 2 x i64> %a, <vscale x 2 x i64> zeroinitializer
+  %out = call <vscale x 2 x i64> @llvm.aarch64.sve.bic.nxv2i64(<vscale x 2 x i1> %pg,
+                                                               <vscale x 2 x i64> %b,
+                                                               <vscale x 2 x i64> %a_z)
+  ret <vscale x 2 x i64> %out
+}
+
 declare <vscale x 16 x i8> @llvm.aarch64.sve.add.nxv16i8(<vscale x 16 x i1>, <vscale x 16 x i8>, <vscale x 16 x i8>)
 declare <vscale x 8 x i16> @llvm.aarch64.sve.add.nxv8i16(<vscale x 8 x i1>, <vscale x 8 x i16>, <vscale x 8 x i16>)
 declare <vscale x 4 x i32> @llvm.aarch64.sve.add.nxv4i32(<vscale x 4 x i1>, <vscale x 4 x i32>, <vscale x 4 x i32>)
 declare <vscale x 2 x i64> @llvm.aarch64.sve.add.nxv2i64(<vscale x 2 x i1>, <vscale x 2 x i64>, <vscale x 2 x i64>)
 
 declare <vscale x 16 x i8> @llvm.aarch64.sve.sub.nxv16i8(<vscale x 16 x i1>, <vscale x 16 x i8>, <vscale x 16 x i8>)
 declare <vscale x 8 x i16> @llvm.aarch64.sve.sub.nxv8i16(<vscale x 8 x i1>, <vscale x 8 x i16>, <vscale x 8 x i16>)
 declare <vscale x 4 x i32> @llvm.aarch64.sve.sub.nxv4i32(<vscale x 4 x i1>, <vscale x 4 x i32>, <vscale x 4 x i32>)