[PowerPC] Check subregister defines before removing XXMFACC/XXMTACC

[lkail]

llvm/test/CodeGen/PowerPC/ppc64-acc-regalloc-bugfix.ll

4–7

Please make adding -ppc-track-subreg-liveness another check-prefix.

[nemanjai]

Accumulator prime and deprime instructions are quite expensive. The logic of this patch seems to be contrary to what we actually want. Namely, this will ensure that we don't remove redundant XXMTACC/XXMFACC pairs if some subregister of the associated accumulator is undefined (or defined in another basic block). If it is undefined, we don't care what its value is so it doesn't seem like a good idea to activate MMA simply because of such poorly written code.
Honestly, anything is likely better than keeping these - including simply defining the undefined register to all zeros using XXLXORz. But of course, we can't simply change this code to zero out the register in case it is actually defined in another block.

Would it be possible to delete the instructions just like we currently do and then use LivePhysRegs to eliminate any uses of dead registers? Basically, what I mean is that if any changes were made in this peephole, we do what is effectively dead instruction elimination as the last thing the peephole does on the entire function.

Doing so would turn the function in the test case into an empty one (which would be the correct thing to do here).

[shchenz]

Do we plan to move forward @qiucf

Deleting the instruction seems can not work for all cases, for example, %r = and 0, undef, this is well defined.

IMO, can we just change the input case? For example, don't store undef(element 2) to memory, we can extract and store element 3, maybe? Storing undef should not be intended by the user and the machineverify failure can exactly tell the user there is something wrong in the source or in compiler optimizations?

[shchenz]

guess this can be abandoned this now? See https://reviews.llvm.org/D139492

llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp

     // of a current candidate marks both the prime and de-prime for removal.
     // This way we ensure we only remove prime/de-prime *pairs* with no
     // intervening uses.
     bool removeAccPrimeUnprime(MachineBasicBlock &MBB) {
       DenseSet<MachineInstr *> InstrsToErase;
       // Initially, none of the acc registers are candidates.
       SmallVector<MachineInstr *, 8> Candidates(
           PPC::UACCRCRegClass.getNumRegs(), nullptr);
+      SmallVector<bool, 32> VsrDefined(32, false);
 
       for (MachineInstr &BBI : MBB.instrs()) {
         unsigned Opc = BBI.getOpcode();
         // If we are visiting a xxmtacc instruction, we add it and its operand
         // register to the candidate set.
         if (Opc == PPC::XXMTACC) {
           Register Acc = BBI.getOperand(0).getReg();
           assert(PPC::ACCRCRegClass.contains(Acc) &&
                  "Unexpected register for XXMTACC");
           Candidates[Acc - PPC::ACC0] = &BBI;
         }
         // If we are visiting a xxmfacc instruction and its operand register is
         // in the candidate set, we mark the two instructions for removal.
         else if (Opc == PPC::XXMFACC) {
           Register Acc = BBI.getOperand(0).getReg();
           assert(PPC::ACCRCRegClass.contains(Acc) &&
                  "Unexpected register for XXMFACC");
-          if (!Candidates[Acc - PPC::ACC0])
+          unsigned Offset = Acc - PPC::ACC0;
+          if (!Candidates[Offset])
             continue;
-          InstrsToErase.insert(&BBI);
-          InstrsToErase.insert(Candidates[Acc - PPC::ACC0]);
+          if (std::all_of(VsrDefined.begin() + Offset * 4,
+                          VsrDefined.begin() + (Offset + 1) * 4,
+                          [](bool Defined) { return Defined; })) {
+            InstrsToErase.insert(&BBI);
+            InstrsToErase.insert(Candidates[Acc - PPC::ACC0]);
+          }
         }
         // If we are visiting an instruction using an accumulator register
         // as operand, we remove it from the candidate set.
         else {
           for (MachineOperand &Operand : BBI.operands()) {
             if (!Operand.isReg())
               continue;
             Register Reg = Operand.getReg();
             if (PPC::ACCRCRegClass.contains(Reg))
               Candidates[Reg - PPC::ACC0] = nullptr;
+
+            if (Operand.isDef()) {
+              if (PPC::ACCRCRegClass.contains(Reg)) {
+                for (int Index = 0; Index < 4; ++Index)
+                  VsrDefined[Reg - PPC::ACC0 + Index] = true;
+              } else if (PPC::VSRpRCRegClass.contains(Reg) &&
+                         Reg < PPC::VSRp16) {
+                VsrDefined[Reg - PPC::VSRp0] = true;
+                VsrDefined[Reg - PPC::VSRp0 + 1] = true;
+              } else if (PPC::VSLRCRegClass.contains(Reg)) {
+                VsrDefined[Reg - PPC::VSL0] = true;
+              }
+            }
           }
         }
       }
 
       for (MachineInstr *MI : InstrsToErase)
         MI->eraseFromParent();
       NumRemovedInPreEmit += InstrsToErase.size();
       return !InstrsToErase.empty();

llvm/test/CodeGen/PowerPC/ppc64-acc-regalloc-bugfix.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs -mtriple powerpc64le-unknown-linux-gnu \
 ; RUN:     -mcpu=pwr10 -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names < %s \
-; RUN:    | FileCheck %s
+; RUN:     | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple powerpc64le-unknown-linux-gnu \
+; RUN:     -mcpu=pwr10 -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names < %s \
+; RUN:     -ppc-track-subreg-liveness | FileCheck %s -check-prefix=SUBTRACK
 
 define void @copy_novsrp() local_unnamed_addr {
 ; CHECK-LABEL: copy_novsrp:
 ; CHECK:       # %bb.0: # %dmblvi_entry
 ; CHECK-NEXT:    xxlxor v2, v2, v2
 ; CHECK-NEXT:    xxlxor vs0, vs0, vs0
 ; CHECK-NEXT:    xxlor vs3, v2, v2
+; CHECK-NEXT:    xxmtacc acc0
+; CHECK-NEXT:    xxmfacc acc0
 ; CHECK-NEXT:    stxv vs1, 0(0)
+;
+; SUBTRACK-LABEL: copy_novsrp:
+; SUBTRACK:       # %bb.0: # %dmblvi_entry
+; SUBTRACK-NEXT:    xxlxor vs3, vs3, vs3
+; SUBTRACK-NEXT:    xxlxor vs0, vs0, vs0
+; SUBTRACK-NEXT:    xxmtacc acc0
+; SUBTRACK-NEXT:    xxmfacc acc0
+; SUBTRACK-NEXT:    stxv vs1, 0(0)
 dmblvi_entry:
   %0 = tail call <512 x i1> @llvm.ppc.mma.assemble.acc(<16 x i8> zeroinitializer, <16 x i8> undef, <16 x i8> undef, <16 x i8> zeroinitializer)
   %1 = tail call { <16 x i8>, <16 x i8>, <16 x i8>, <16 x i8> } @llvm.ppc.mma.disassemble.acc(<512 x i1> %0)
   %2 = extractvalue { <16 x i8>, <16 x i8>, <16 x i8>, <16 x i8> } %1, 2
   store <16 x i8> %2, <16 x i8>* null, align 1
   unreachable
 }
 
 declare <512 x i1> @llvm.ppc.mma.assemble.acc(<16 x i8>, <16 x i8>, <16 x i8>, <16 x i8>)
 declare { <16 x i8>, <16 x i8>, <16 x i8>, <16 x i8> } @llvm.ppc.mma.disassemble.acc(<512 x i1>)