[fastregalloc] Fix bug when undef value is tied to def.

[LuoYuanke]

Summary

If the tied use is undef value, fastregalloc should free the def
register. There is no reload needed for the undef value.

[LuoYuanke]

Remove unused commends.

[arsenm]

llvm/lib/CodeGen/RegAllocFast.cpp

1115

Might as well pass in MachineOperand since that's what you already have at the use points

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

6–27

Can drop the IR section if you drop the block names and IR value references

42

Generate full checks?

[MatzeB]

llvm/lib/CodeGen/RegAllocFast.cpp

1115–1122

I think this would be easier to understand for readers if you go for a different name and semantic. Despite the use being "undef" it's still a tied operand after all. You just want to change the algorithm to ignore those, but for that I think it's clearer to state that within the algorithm.

So how about:

1128–1141

In the above spirit change this to:

1241–1242

1281

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

29–52

On top of dropping the IR you can probably simplify like this?

[LuoYuanke]

Address Arsenault and Braun's comments.

[MatzeB]

LGTM with nitpick addressed.

[MatzeB]

llvm/lib/CodeGen/RegAllocFast.cpp

1118

Please avoid auto when the type isn't immediately visible within the same line, it makes the code easier to read. See https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

[LuoYuanke]

Address Braun's comments.

[craig.topper]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

Was this a synthetic example? It shouldn't be possible to generate a PXORrr like this before register allocation.

[LuoYuanke]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

I use the following C code to generate the test case. However compiler would generate "%0:vr128 = V_SET0" instead of "%0:vr128 = PXORrr undef %0, undef %0". The real case that I encounter is to generate code to zero the stack slot for AMX configure register. I use "PXORrr" to zero the stack slot, I can change it to "V_SET0" to avoid the issue in fast regalloc. However I think it expose an issue in fast regalloc, so I create a patch for it.

#include <immintrin.h>

void foo() {
  __m128 vec[4];
  __m128 m = {0, 0};

  vec[0] = m;
  vec[1] = m;
  vec[2] = m;
  vec[3] = m;
}

[craig.topper]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

Thanks. I recommend using V_SET0. If you create the PXOR while in SSA form the source and dest will need different vregs. I think the two address instruction pass will change the tied one to match the dest, but it won't change the other one. Then the register allocator is not obligated to give the 2 sources the same register. The untied source will likely always end up with xmm0. If the other source isn't xmm0 it won't be recognized as a zero idiom by the hardware. V_SET0 exists to work around all of that.

[craig.topper]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

Actually its worse than that. Not only will it not be recognized by the hardware. It won't produce 0. It will produce a random value since the register contents don't match.

[LuoYuanke]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

Thanks, Craig :) I'll change the code.

[MatzeB]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

I think the two address instruction pass will change the tied one to match the dest, but it won't change the other one.

I don't think TwoAddressInstruction does that. I can't find the relevant code right now, but at least the explanation in MachineOperand.h for the undef flag says:

/// Note that an instruction may have multiple <undef> operands referring to
/// the same register.  In that case, the instruction may depend on those
/// operands reading the same dont-care value.  For example:
///
///   %1 = XOR undef %2, undef %2

[craig.topper]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

I could be wrong. I think the relevant code is in TwoAddressInstructionPass::collectTiedOperands. At first glance it doesn't look like it's trying to keep other operands the same when it rewrites undef tied operands.

[craig.topper]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

I hacked the test and ran the twoaddressinstruction pass

# *** IR Dump Before Two-Address instruction pass (twoaddressinstruction) ***:
# Machine code for function foo: IsSSA, NoPHIs, TracksLiveness
Frame Objects:
  fi#0: size=64, align=16, at location [SP+8]
  fi#1: size=16, align=16, at location [SP+8]

bb.0.entry:
  %1:vr128 = PXORrr undef %0:vr128(tied-def 0), undef %0:vr128
  MOVAPSmr %stack.1, 1, $noreg, 0, $noreg, %1:vr128
  MOVAPSmr %stack.0, 1, $noreg, 0, $noreg, %1:vr128
  MOVAPSmr %stack.0, 1, $noreg, 16, $noreg, %1:vr128
  MOVAPSmr %stack.0, 1, $noreg, 32, $noreg, %1:vr128
  MOVAPSmr %stack.0, 1, $noreg, 48, $noreg, killed %1:vr128
  RET 0

# End machine code for function foo.

********** REWRITING TWO-ADDR INSTRS **********
********** Function: foo
                rewrite undef:  %1:vr128 = PXORrr undef %1:vr128(tied-def 0), undef %0:vr128
        %1:vr128 = PXORrr undef %1:vr128(tied-def 0), undef %0:vr128

It creates a PXORrr with different source registers.

[MatzeB]

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

28

Interesting! Guess twoaddressinstruction is buggy then given how we define the undef flag and people just got used to use the workaround with pseudo instructions instead...

llvm/lib/CodeGen/RegAllocFast.cpp

void RegAllocFast::allocateInstruction(MachineInstr &MI) {
   //   the pre-assignment.
   // - The "free def operands" step has to come last instead of first for tied
   //   operands and early-clobbers.
 
   UsedInInstr.clear();
   RegMasks.clear();
   BundleVirtRegsMap.clear();
 
+  auto TiedOpIsUndef = [&](const MachineOperand &MO, unsigned Idx) {
+    assert(MO.isTied());
+    unsigned TiedIdx = MI.findTiedOperandIdx(Idx);
+    const MachineOperand &TiedMO = MI.getOperand(TiedIdx);
+    return TiedMO.isUndef();
+  };
   // Scan for special cases; Apply pre-assigned register defs to state.
   bool HasPhysRegUse = false;
   bool HasRegMask = false;
   bool HasVRegDef = false;
   bool HasDef = false;
   bool HasEarlyClobber = false;
   bool NeedToAssignLiveThroughs = false;
-  for (MachineOperand &MO : MI.operands()) {
+  for (unsigned I = 0; I < MI.getNumOperands(); ++I) {
+    MachineOperand &MO = MI.getOperand(I);
     if (MO.isReg()) {
       Register Reg = MO.getReg();
       if (Reg.isVirtual()) {
         if (MO.isDef()) {
           HasDef = true;
           HasVRegDef = true;
           if (MO.isEarlyClobber()) {
             HasEarlyClobber = true;
             NeedToAssignLiveThroughs = true;
           }
-          if (MO.isTied() || (MO.getSubReg() != 0 && !MO.isUndef()))
+          if ((MO.isTied() && !TiedOpIsUndef(MO, I)) ||
+              (MO.getSubReg() != 0 && !MO.isUndef()))
             NeedToAssignLiveThroughs = true;
         }
       } else if (Reg.isPhysical()) {
         if (!MRI->isReserved(Reg)) {
           if (MO.isDef()) {
             HasDef = true;
             bool displacedAny = definePhysReg(MI, Reg);
             if (MO.isEarlyClobber())
if (HasVRegDef) {
           // Tie-break rule: operand index.
           return I0 < I1;
         });
 
         for (uint16_t OpIdx : DefOperandIndexes) {
           MachineOperand &MO = MI.getOperand(OpIdx);
           LLVM_DEBUG(dbgs() << "Allocating " << MO << '\n');
           unsigned Reg = MO.getReg();
-          if (MO.isEarlyClobber() || MO.isTied() ||
+          if (MO.isEarlyClobber() ||
+              (MO.isTied() && !TiedOpIsUndef(MO, OpIdx)) ||
               (MO.getSubReg() && !MO.isUndef())) {
             defineLiveThroughVirtReg(MI, OpIdx, Reg);
           } else {
             defineVirtReg(MI, OpIdx, Reg);
           }
         }
       } else {
         // Assign virtual register defs.
         for (unsigned I = 0, E = MI.getNumOperands(); I < E; ++I) {
           MachineOperand &MO = MI.getOperand(I);
           if (!MO.isReg() || !MO.isDef())
             continue;
           Register Reg = MO.getReg();
           if (Reg.isVirtual())
             defineVirtReg(MI, I, Reg);
         }
       }
     }
 
     // Free registers occupied by defs.
     // Iterate operands in reverse order, so we see the implicit super register
     // defs first (we added them earlier in case of <def,read-undef>).
-    for (MachineOperand &MO : llvm::reverse(MI.operands())) {
+    for (signed I = MI.getNumOperands() - 1; I >= 0; --I) {
+      MachineOperand &MO = MI.getOperand(I);
       if (!MO.isReg() || !MO.isDef())
         continue;
 
       // subreg defs don't free the full register. We left the subreg number
       // around as a marker in setPhysReg() to recognize this case here.
       if (MO.getSubReg() != 0) {
         MO.setSubReg(0);
         continue;
       }
 
       assert((!MO.isTied() || !isClobberedByRegMasks(MO.getReg())) &&
              "tied def assigned to clobbered register");
 
       // Do not free tied operands and early clobbers.
-      if (MO.isTied() || MO.isEarlyClobber())
+      if ((MO.isTied() && !TiedOpIsUndef(MO, I)) || MO.isEarlyClobber())
         continue;
       Register Reg = MO.getReg();
       if (!Reg)
         continue;
       assert(Reg.isPhysical());
       if (MRI->isReserved(Reg))
         continue;
       freePhysReg(Reg);

llvm/test/CodeGen/X86/fastregalloc-tied-undef.mir

+# RUN: llc -mtriple=x86_64-- -run-pass=regallocfast -o - %s | FileCheck %s
+
+# If the tied use is undef value, fastregalloc should free the def register.
+# There is no reload needed for the undef value.
+...
+---
+name:            foo
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: vr128 }
+frameInfo:
+  maxAlignment:    16
+stack:
+  - { id: 0, size: 64, alignment: 16 }
+  - { id: 1, size: 16, alignment: 16 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    ; CHECK-LABEL: bb.0.entry
+    ; CHECK-NEXT: renamable $xmm0 = PXORrr undef renamable $xmm0, undef renamable $xmm0
+    ; CHECK-NEXT: MOVAPSmr %stack.1, 1, $noreg, 0, $noreg, renamable $xmm0
+    ; CHECK-NEXT: MOVAPSmr %stack.0, 1, $noreg, 0, $noreg, renamable $xmm0
+    ; CHECK-NEXT: MOVAPSmr %stack.0, 1, $noreg, 16, $noreg, renamable $xmm0
+    ; CHECK-NEXT: MOVAPSmr %stack.0, 1, $noreg, 32, $noreg, renamable $xmm0
+    ; CHECK-NEXT: MOVAPSmr %stack.0, 1, $noreg, 48, $noreg, killed renamable $xmm0
+
+    %0:vr128 = PXORrr undef %0, undef %0
+    MOVAPSmr %stack.1, 1, $noreg, 0, $noreg, %0
+    MOVAPSmr %stack.0, 1, $noreg, 0, $noreg, %0
+    MOVAPSmr %stack.0, 1, $noreg, 16, $noreg, %0
+    MOVAPSmr %stack.0, 1, $noreg, 32, $noreg, %0
+    MOVAPSmr %stack.0, 1, $noreg, 48, $noreg, killed %0
+    RET 0
+
+...