[AArch64][SVE] Fix crash for DestructiveBinaryComm zero merging

[Allen]

Summary

This fix is similar to D124325, and I find the DestructiveBinaryComm
operation type also may be allocated same register, so insert the LSL.

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

[Allen]

ping ?

[paulwalker-arm]

This is what I was referring to during my review of D124325 whereby I was suggesting your fix was more generic than you thought and could be the way to solve all such problems for all the destructive types that have two or more operands.

That being said, I have no problem with build it up one destructive type at a time.

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

561

This being an || chain already in brackets I doubt you need these inner brackets.

[Allen]

In D141471#4058322, @paulwalker-arm wrote:

This is what I was referring to during my review of D124325 whereby I was suggesting your fix was more generic than you thought and could be the way to solve all such problems for all the destructive types that have two or more operands.

That being said, I have no problem with build it up one destructive type at a time.

Thanks, I'll follow up to deal with other destructive types in the next step.

[Allen]

Add inner brackets according comment

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

561

I think you've misunderstood my comment and have also introduced a bug. I meant there's no need for any new brackets i.e. you can write (DOPRegIsUnique || AArch64::DestructiveBinary == DType || AArch64::DestructiveBinaryComm == DType) && ...

Also with the new update you've incorrectly replaced the DestructiveBinaryComm comparison within an assignment of DType.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

561

I've pushed a fix.

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

561

Apply your comment, thanks

561

Oh, sorry. Thanks very much

[Allen]

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

561

Oh, sorry. Thanks very much

561

Oh, sorry. Thanks very much

561

I see your commit 78ba3e785cd

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

bool AArch64ExpandPseudo::expand_DestructiveOp(
 
   //
   // Create the destructive operation (if required)
   //
   MachineInstrBuilder PRFX, DOP;
   if (FalseZero) {
     // If we cannot prefix the requested instruction we'll instead emit a
     // prefixed_zeroing_mov for DestructiveBinary.
-    assert((DOPRegIsUnique || AArch64::DestructiveBinary == DType) &&
+    assert((DOPRegIsUnique || ((DType == AArch64::DestructiveBinary) ||
+                               (DType = AArch64::DestructiveBinaryComm))) &&
            "The destructive operand should be unique");
     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;
 
     // Create the additional LSL to zero the lanes when the DstReg is not
     // unique. Zeros the lanes in z0 that aren't active in p0 with sequence
     // movprfx z0.b, p0/z, z0.b; lsl z0.b, p0/m, z0.b, #0;
-    if (DType == AArch64::DestructiveBinary && !DOPRegIsUnique) {
+    if (((DType == AArch64::DestructiveBinary) ||
+         (DType == AArch64::DestructiveBinaryComm)) &&
+        !DOPRegIsUnique) {
       BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(LSLZero))
           .addReg(DstReg, RegState::Define)
           .add(MI.getOperand(PredIdx))
           .addReg(DstReg)
           .addImm(0);
     }
   } else if (DstReg != MI.getOperand(DOPIdx).getReg()) {
     assert(DOPRegIsUnique && "The destructive operand should be unique");

llvm/test/CodeGen/AArch64/sve-intrinsics-int-binaryComm-merging.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=aarch64 -mattr=+sve -mattr=+use-experimental-zeroing-pseudos -run-pass=aarch64-expand-pseudo %s -o - | FileCheck %s
+
+# Should create an additional LSL to zero the lanes as the DstReg is not unique
+
+--- |
+  define <vscale x 4 x float> @fmul_float_zero(<vscale x 4 x i1> %pg, <vscale x 4 x float> %a){
+    %a_z = select <vscale x 4 x i1> %pg, <vscale x 4 x float> %a, <vscale x 4 x float> zeroinitializer
+    %out = call <vscale x 4 x float> @llvm.aarch64.sve.fmul.nxv4f32(<vscale x 4 x i1> %pg, <vscale x 4 x float> %a_z, <vscale x 4 x float> %a_z)
+    ret <vscale x 4 x float> %out
+  }
+
+  declare <vscale x 4 x float> @llvm.aarch64.sve.fmul.nxv4f32(<vscale x 4 x i1>, <vscale x 4 x float>, <vscale x 4 x float>)
+...
+---
+name:            fmul_float_zero
+alignment:       4
+tracksRegLiveness: true
+tracksDebugUserValues: true
+registers:       []
+liveins:
+  - { reg: '$p0', virtual-reg: '' }
+  - { reg: '$z0', virtual-reg: '' }
+body:             |
+  bb.0 (%ir-block.0):
+    liveins: $p0, $z0
+
+    ; CHECK-LABEL: name: fmul_float_zero
+    ; CHECK: liveins: $p0, $z0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: BUNDLE implicit-def $z0, implicit-def $q0, implicit-def $d0, implicit-def $s0, implicit-def $h0, implicit-def $b0, implicit-def $z0_hi, implicit $p0, implicit $z0 {
+    ; CHECK-NEXT:   $z0 = MOVPRFX_ZPzZ_S $p0, $z0
+    ; CHECK-NEXT:   $z0 = LSL_ZPmI_S renamable $p0, internal $z0, 0
+    ; CHECK-NEXT:   $z0 = FMUL_ZPmZ_S renamable $p0, internal killed $z0, internal killed renamable $z0
+    ; CHECK-NEXT: }
+    ; CHECK-NEXT: RET undef $lr, implicit $z0
+    renamable $z0 = nnan ninf nsz arcp contract afn reassoc FMUL_ZPZZ_ZERO_S renamable $p0, killed renamable $z0, renamable $z0
+    RET_ReallyLR implicit $z0
+...