assigned to @MatzeB

As far as I can tell, in this example ExpandPostRA::LowerCopy
incorrectly concludes that %al is not live over an instruction its
expanding, and ends up clobbering %al.

I think there are two indpendent issues here:

In MachineBasicBlock::computeRegisterLiveness, we need something like
this:

diff --git a/lib/CodeGen/MachineBasicBlock.cpp b/lib/CodeGen/MachineBasicBlock.cpp
index 8060308..0d3daf3 100644
--- a/lib/CodeGen/MachineBasicBlock.cpp
+++ b/lib/CodeGen/MachineBasicBlock.cpp
@@ -1208,7 +1208,9 @@ MachineBasicBlock::computeRegisterLiveness(const TargetRegisterInfo *TRI,
return (Analysis.Reads) ?
LQR_Live : LQR_OverlappingLive;

•  else if (Analysis.Clobbers || Analysis.Defines)
  

•  else if (Analysis.Clobbers && !Analysis.Defines)
  

•    return LQR_Unknown;
  

•  else if (Analysis.Defines)
     // Defined (but not read) therefore cannot have been live.
     return LQR_Dead;
  

  }

because otherwise we'll conclude, e.g., AX is killed by "%AH =
...", when the AL half of AX could still be live.

Secondly, I think there's a bug in
MachineOperandIteratorBase::analyzePhysReg -- I think it should check
if the defined register == MOReg is a super register of Reg.

diff --git a/lib/CodeGen/MachineInstrBundle.cpp b/lib/CodeGen/MachineInstrBundle.cpp
index f6e45a4..5e474c3 100644
--- a/lib/CodeGen/MachineInstrBundle.cpp
+++ b/lib/CodeGen/MachineInstrBundle.cpp
@@ -310,7 +310,7 @@ MachineOperandIteratorBase::analyzePhysReg(unsigned Reg,
if (!MOReg || !TargetRegisterInfo::isPhysicalRegister(MOReg))
continue;

• bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg);

• bool IsRegOrSuperReg = TRI->isSuperRegisterEq(Reg, MOReg);
  bool IsRegOrOverlapping = MOReg == Reg || TRI->regsOverlap(MOReg, Reg);

  if (IsRegOrSuperReg && MO.readsReg()) {

I'm not sure if the following change is correct (I'd need to dig some more to understand):

• bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg);

• bool IsRegOrSuperReg = TRI->isSuperRegisterEq(Reg, MOReg);

But the name of the variable should change to IsSuperReg if the change is correct :-)

Could you elaborate on why you think MOReg == Reg is incorrect?

I'm not sure if the following change is correct (I'd need to dig some more
to understand):

• bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg);

• bool IsRegOrSuperReg = TRI->isSuperRegisterEq(Reg, MOReg);

But the name of the variable should change to IsSuperReg if the change is
correct :-)

This is calling isSuperRegisterEq, which contains the MOReg == Reg IIUC.

Could you elaborate on why you think MOReg == Reg is incorrect?

The important semantic change is in the order of the operands, so IIUC this change makes the code equivalent to

bool IsRegOrSuperReg = MOReg == Reg || TRI->isSubRegister(MOReg, Reg);

(Lots of IIUC's there, because this is the first time I'm reading this code :) )

A note about reproducibility: I could not reproduce this directly using clang on OSX. But I can reproduce this by running

./bin/llc -O3 small.ll && clang -m32 small.s -o small

(with the attached small.ll)

Is it possible that the exact pipeline differs between OSX and linux?

small.ll

The following testcase likely reveals the same issue(s):

$ clang-trunk -v
clang version 3.8.0 (trunk 254485)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.6.4
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.7
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.7.3
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.8.4
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9.3
Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/5.2.1
Selected GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.9
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Candidate multilib: x32;@MX32
Selected multilib: .;@m64
$
$ clang-trunk -m32 -O2 small.c
$ ./a.out
Aborted (core dumped)
$
$ clang-3.7.0 -m32 -O2 small.c
$ ./a.out
$

int a, b, c, e, *f = &e, k;
char d = 2, g;

void
fn2 (int p1, int p2)
{
d--;
c++;
k = p1 ^ b;
if (c)
{
d--;
g = p2;
if (!f)
__builtin_abort ();
}
else
for (;;)
;
}

int
main ()
{
fn2 (a && a, a);

if (d)
__builtin_abort ();

return 0;
}

I think the following bugs all trigger similar bugs:
https://llvm.org/bugs/show_bug.cgi?id=25033
https://llvm.org/bugs/show_bug.cgi?id=24535
https://llvm.org/bugs/show_bug.cgi?id=24991
https://llvm.org/bugs/show_bug.cgi?id=24992
https://llvm.org/bugs/show_bug.cgi?id=25201

MachineOperandIteratorBase::analyzePhysReg and MachineBasicBlock::computeRegisterLiveness aren't quite in sync with each other when it comes to super-registers and sub-registers.

I think the following is also working around the same issue:
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140317/209514.html

Sanjoy: I tried out your proposed fix and it doesn't seem to address the other issues. Have you made any progress since then? I also tried a few things but the logic of this code is giving me a headache.

Here's a workaround for now:
http://reviews.llvm.org/D15198

I haven't spent more time looking at this. But this is what I
understand about the situation currently: there is mismatch between
the definition of "clobber" between computeRegisterLiveness and
analyzePhysReg.

analyzePhysReg returns Clobber if the register Reg is only partially
defined by the instruction. (IsRegOrOverlapping will be true for such
a case).

computeRegisterLiveness thinks a register is dead if it it finds a
clobber of the register before the "Before" iterator:

  if (Analysis.Kills || Analysis.Clobbers)
    // Register killed, so isn't live.
    return LQR_Dead;

This means we fail see that RAX (say) is partially live over FOO in

%eax = ... ;; (1)
FOO ;; (2)
use(%eax) ;; (3)

analyzePhysReg says %rax is clobbered by (1). computeRegisterLiveness
therefore concludes %rax is not live over (2). However, %rax is
partially live over (2), and hence is not (fully) free to use as a
scratch register at (2).

The fix is either to

• Change computeRegisterLiveness to not return Clobber for partially
  defined registers

• Change analyzePhysReg to not assume "Dead" for registers that are
  clobbered

The workaround is now checked in. We should leave these bugs open and fix the core issue.

Sanjoy was right in that computeRegisterLiveness() interpreted the meaning of Clobbers wrong.

I figured that part of the problem was that the meaning of Clobbers as clobbered by the regmask or partially defined wasn't particularily usefull for the usecase (you want to know about full clobbers or defines).

I cleaned up/rewrote the code in http://reviews.llvm.org/D15320

*** Bug #24909 has been marked as a duplicate of this bug. ***

*** Bug #25365 has been marked as a duplicate of this bug. ***

*** Bug #25366 has been marked as a duplicate of this bug. ***

*** Bug llvm/llvm-bugzilla-archive#25201 has been marked as a duplicate of this bug. ***

I tried the reproducers in all the bugs mentioned in comment #​7 and they all work fine with my patch (clang -O3 -m32 xxx.c on OS X).

Should be fine now that http://reviews.llvm.org/D15320 landed.

mentioned in issue llvm/llvm-bugzilla-archive#25201