Check for register clobbers when merging a vreg live range with a reserved physreg in RegisterCoalescer.

[jyknight]

Summary

Previously, we only checked for clobbers when merging into a READ of
the physreg, but not when merging from a WRITE to the physreg.

[MatzeB]

Makes sense, I am surprised this didn't hit us before.

Could you create a .mir based test that just runs the coalescer instead of the .ll one?

[jyknight]

Added MIR test case to AArch64/regcoal-physreg.mir.

[jyknight]

In D28527#641748, @MatzeB wrote:

Makes sense, I am surprised this didn't hit us before.

Could you create a .mir based test that just runs the coalescer instead of the .ll one?

I noted the existing MIR test in AArch64/regcoal-physreg.mir, and added another test-case there.

I also noticed while writing this new test case that MachineRegisterInfo::isConstantPhysReg treats any register without a visible def in the function as "constant". And thus, the newly added test case doesn't work -- it will coalesce -- without the "%x18 = COPY %xzr" line.

I'm not sure this is correct behavior, since it *also* fails to take into account regmasks, but I also don't know if there's a situation where it would actually matter -- in the actual case I'm looking at, the inlineasm is a def of the register, so that's fine.

[qcolombet]

test/CodeGen/AArch64/regcoal-physreg.mir

77 ↗

(On Diff #84126)

That sounds like a bug to me.
I would have expected the constant attribute to be decided by the target, i.e., not something that depends on the presence of a definition.

Could you investigate please?

[MatzeB]

LGTM
The "no-def+!allocatable => constant" rule has been there for a long time in MachineRegisterInfo::isConstantPhysReg(), not sure we need to check/question it as part of this commit...

[qcolombet]

The "no-def+!allocatable => constant" rule has been there for a long time in MachineRegisterInfo::isConstantPhysReg()

Oh, I missed the !allocatable in the description of the problem.
Yeah, no need to investigate then.

llvm/trunk/test/CodeGen/AArch64/regcoal-physreg.mir

78

You could use something else than x18 to avoid the !allocatable case that Matthias mentioned, right?
What I am saying is that the comment is a bit misleading as it is, because this is expected when register are not allocatable.

[jyknight]

llvm/trunk/test/CodeGen/AArch64/regcoal-physreg.mir

78

The bug only exhibits for reserved registers in the first place (and only, then, if it is also clobbered by a call), so another register wouldn't be appropriate.

llvm/trunk/lib/CodeGen/RegisterCoalescer.cpp

for (MCRegUnitIterator UI(DstReg, TRI); UI.isValid(); ++UI) {
         if (!MRI->isReserved(*RI))
           return false;
       }
       if (RHS.overlaps(LIS->getRegUnit(*UI))) {
         DEBUG(dbgs() << "\t\tInterference: " << PrintRegUnit(*UI, TRI) << '\n');
         return false;
       }
     }
+
+    // We must also check for overlaps with regmask clobbers.
+    BitVector RegMaskUsable;
+    if (LIS->checkRegMaskInterference(RHS, RegMaskUsable) &&
+        !RegMaskUsable.test(DstReg)) {
+      DEBUG(dbgs() << "\t\tRegMask interference\n");
+      return false;
+    }
   }
 
   // Skip any value computations, we are not adding new values to the
   // reserved register.  Also skip merging the live ranges, the reserved
   // register live range doesn't need to be accurate as long as all the
   // defs are there.
 
   // Delete the identity copy.
if (!MRI->isConstantPhysReg(DstReg)) {
       SlotIndexes *Indexes = LIS->getSlotIndexes();
       for (SlotIndex SI = Indexes->getNextNonNullIndex(DestRegIdx);
            SI != CopyRegIdx; SI = Indexes->getNextNonNullIndex(SI)) {
         MachineInstr *MI = LIS->getInstructionFromIndex(SI);
         if (MI->readsRegister(DstReg, TRI)) {
           DEBUG(dbgs() << "\t\tInterference (read): " << *MI);
           return false;
         }
-
-        // We must also check for clobbers caused by regmasks.
-        for (const auto &MO : MI->operands()) {
-          if (MO.isRegMask() && MO.clobbersPhysReg(DstReg)) {
-            DEBUG(dbgs() << "\t\tInterference (regmask clobber): " << *MI);
-            return false;
-          }
-        }
       }
     }
 
     // We're going to remove the copy which defines a physical reserved
     // register, so remove its valno, etc.
     DEBUG(dbgs() << "\t\tRemoving phys reg def of " << DstReg << " at "
           << CopyRegIdx << "\n");
 

llvm/trunk/test/CodeGen/AArch64/regcoal-physreg.mir

 # RUN: llc -mtriple=aarch64-apple-ios -run-pass=simple-register-coalescing %s -o - | FileCheck %s
 --- |
+  declare void @f2()
+
   define void @func() { ret void }
 ...
 ---
 # Check coalescing of COPYs from reserved physregs.
 # CHECK-LABEL: name: func
 name: func
 registers:
   - { id: 0, class: gpr32 }
   - { id: 1, class: gpr64 }
   - { id: 2, class: gpr64 }
   - { id: 3, class: gpr32 }
   - { id: 4, class: gpr64 }
   - { id: 5, class: gpr32 }
   - { id: 6, class: xseqpairsclass }
+  - { id: 7, class: gpr64 }
 body: |
   bb.0:
     ; We usually should not coalesce copies from allocatable physregs.
     ; CHECK: %0 = COPY %w7
     ; CHECK: STRWui %0, %x1, 0
     %0 = COPY %w7
     STRWui %0, %x1, 0
 
bb.0:
 
     ; Only coalesce when the source register is reserved as a whole (this is
     ; a limitation of the current code which cannot update liveness information
     ; of the non-reserved part).
     ; CHECK: %6 = COPY %xzr_x0
     ; CHECK: HINT 0, implicit %6
     %6 = COPY %xzr_x0
     HINT 0, implicit %6
+
+    ; It is not fine to coalesce copies from reserved physregs when they are
+    ; clobbered by the regmask on a call.
+    ; CHECK: %7 = COPY %x18
+    ; CHECK: BL @f2, csr_aarch64_aapcs, implicit-def dead %lr, implicit %sp, implicit-def %sp
+    ; CHECK: STRXui %7, %x1, 0
+
+    ; Need a def of x18 so that it's not deduced as "constant".
+    %x18 = COPY %xzr
+    %7 = COPY %x18
+    BL @f2, csr_aarch64_aapcs, implicit-def dead %lr, implicit %sp, implicit-def %sp
+    STRXui %7, %x1, 0
 ...

llvm/trunk/test/CodeGen/SPARC/register-clobber.ll

+; RUN: llc -march=sparc < %s | FileCheck %s
+
+;; Verify that g1 (the output of first asm) is properly understood to
+;; be clobbered by the call instruction, and moved out of the way
+;; before it. (NOTE: remember delay slot; mov executes before call)
+
+; CHECK-LABEL: test1:
+; CHECK: ta       9
+; CHECK: call dosomething
+; CHECK: mov      %g1, %i0
+
+define i32 @test1() nounwind {
+entry:
+  %0 = tail call i32 asm sideeffect "ta $1", "={r1},i"(i32 9) nounwind
+  tail call void @dosomething() nounwind
+  ret i32 %0
+}
+
+;; Also check using the value.
+; CHECK-LABEL: test2:
+; CHECK: ta       9
+; CHECK: call dosomething
+; CHECK: mov      %g1, %i0
+; CHECK: mov      %i0, %g1
+; CHECK: ta       10
+
+define void @test2() local_unnamed_addr nounwind {
+entry:
+  %0 = tail call i32 asm sideeffect "ta $1", "={r1},i"(i32 9) nounwind
+  tail call void @dosomething() nounwind
+  tail call void asm sideeffect "ta $0", "i,{r1}"(i32 10, i32 %0) nounwind
+  ret void
+}
+
+declare void @dosomething() local_unnamed_addr nounwind