[RegUsageInfoCollector] Don't assume the alias of a defined reg is always already in the set.

[jonpa]

Summary

As the test case in https://bugs.llvm.org/show_bug.cgi?id=36587 discovered, it is wrong to assume that all the aliases of the defined register in the *current function* is already present in the UsedPhysRegsMask.

This patch changes this so that any definition in the current function of a phys-reg always results in all its aliases inserted into the set of defined registers.

[qcolombet]

I am not sure I like the patch in the sense that the comment for UsedPhysRegsMask (in ’MachineRegisterInfo.h) explicitly said that it should contain all the aliases.

So either, we change this assumption (which that patch does) and we update the patch to change the comment *and* we make sure it does not break anything else if other targets start to not put the aliases in that set, or we fix that target to add the aliases when it creates the set.

From a high level point of view, it looks riskier to take the first approach. Can't we change the code that set this mask in SystemZ?

@jonpa from the PR, I didn't get if the mask was "wrong" before RegCollectUsage starts or if it is RegCollectUsage that fails to update it. IIRC, RegCollectUsage does not change UsedPhysRegsMask, so I was assuming the state was wrong to begin with.

[jonpa]

In D45157#1075960, @qcolombet wrote:

I am not sure I like the patch in the sense that the comment for UsedPhysRegsMask (in ’MachineRegisterInfo.h) explicitly said that it should contain all the aliases.

/// UsedPhysRegMask - Additional used physregs including aliases.                                                                                                                                                              
/// This bit vector represents all the registers clobbered by function calls.                                                                                                                                                  
BitVector UsedPhysRegMask;

I read this not as all aliases of all registers are included, but merely those actually clobbered. In the test case, fn1 clobbers %r0l, so %r0l and it's aliases (%r0d and %r0q) are also in UsedPhysRegMask. However, %r0h is *not* clobbered (the other subreg of %r0d).

So either, we change this assumption (which that patch does) and we update the patch to change the comment *and* we make sure it does not break anything else if other targets start to not put the aliases in that set, or we fix that target to add the aliases when it creates the set.

From a high level point of view, it looks riskier to take the first approach. Can't we change the code that set this mask in SystemZ?

@jonpa from the PR, I didn't get if the mask was "wrong" before RegCollectUsage starts or if it is RegCollectUsage that fails to update it. IIRC, RegCollectUsage does not change UsedPhysRegsMask, so I was assuming the state was wrong to begin with.

My understanding is that when RegInfoUsageCollector looks at @fn1, it only adds %r0l (and superregs), but not %r0h. @fn1 only clobbers %r0l. @fn2 however does clobber %r0d, which is already in the set after visiting @fn1. So here it is not true that all aliases of %r0d are in the set!

I am not aware that the backend should set this mask somehow, I have so far assumed that it is the RegCollectUsage/Propagate passes that sets the call regmask.

Perhaps we should change the comment to "... including clobbered aliases..." ?

[qcolombet]

You're right, re-reading the comment indeed, not all the aliases are here (nor should they be!).

test/CodeGen/SystemZ/ipra.ll

15

Nit: Please use opt -instnamer to get rid of the implicit variables

[jonpa]

Thanks for review.

r331509.

lib/CodeGen/RegUsageInfoCollector.cpp

   // Scan all the physical registers. When a register is defined in the current
   // function set it and all the aliasing registers as defined in the regmask.
   for (unsigned PReg = 1, PRegE = TRI->getNumRegs(); PReg < PRegE; ++PReg) {
-    // If a register is in the UsedPhysRegsMask set then mark it as defined.
-    // All it's aliases will also be in the set, so we can skip setting
-    // as defined all the aliases here.
-    if (UsedPhysRegsMask.test(PReg)) {
-      SetRegAsDefined(PReg);
-      continue;
-    }
     // If a register is defined by an instruction mark it as defined together
     // with all it's aliases.
     if (!MRI->def_empty(PReg)) {
       for (MCRegAliasIterator AI(PReg, TRI, true); AI.isValid(); ++AI)
         SetRegAsDefined(*AI);
+      continue;
     }
+    // If a register is in the UsedPhysRegsMask set then mark it as defined.
+    // All clobbered aliases will also be in the set, so we can skip setting
+    // as defined all the aliases here.
+    if (UsedPhysRegsMask.test(PReg))
+      SetRegAsDefined(PReg);
   }
 
   if (!TargetFrameLowering::isSafeForNoCSROpt(F)) {

test/CodeGen/SystemZ/ipra.ll

+; Test that the set of used phys regs used by interprocedural register
+; allocation is correct for a test case where the called function (@fn2)
+; itself has a call (to @fn1). @fn1 defines %r0l, while @fn2 defines
+; %r0d. The RegUsageInfo for @fn2 must include %r0h.
+;
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -enable-ipra -print-regusage 2>&1 < %s \
+; RUN:   | FileCheck %s
+;
+; CHECK: fn2 Clobbered Registers: {{.*}} $r0h
+
+@h = external global [0 x i32], align 4
+@n = external global i32*, align 8
+
+define void @fn1() {
+  br label %1
+
+; <label>:1:                                      ; preds = %1, %0
+  %2 = getelementptr inbounds [0 x i32], [0 x i32]* @h, i64 0, i64 undef
+  %3 = load i32, i32* %2
+  store i32 %3, i32* undef
+  br label %1
+}
+
+define void @fn2() {
+  br label %1
+
+; <label>:1:                                      ; preds = %0
+  br i1 undef, label %2, label %3
+
+; <label>:2:                                      ; preds = %1
+  store i32* null, i32** @n
+  unreachable
+
+; <label>:3:                                      ; preds = %1
+  call void @fn1()
+  unreachable
+}
+
+define void @main() {
+  call void @fn2()
+  ret void
+}
+