The following code is miscompiled by the current clang trunk on x86_64-linux-gnu at -O2 and -O3 in 32-bit mode (but not in 64-bit mode). This is a regression from 3.7.x. It seems different from PR 24991, which disappears with -g enabled. $ clang-trunk -v clang version 3.8.0 (trunk 249170) Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /usr/local/tools/bin Found candidate GCC installation: /usr/lib/gcc/i686-linux-gnu/4.9 Found candidate GCC installation: /usr/lib/gcc/i686-linux-gnu/4.9.2 Found candidate GCC installation: /usr/lib/gcc/i686-linux-gnu/5.1.0 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.4 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/4.4.7 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.2 Found candidate GCC installation: /usr/lib/gcc/x86_64-linux-gnu/5.1.0 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 -m64 -O2 small.c; ./a.out $ clang-trunk -m32 -Os small.c; ./a.out $ clang-3.7.0 -m32 -O2 small.c; ./a.out $ $ clang-trunk -m32 -O2 small.c $ ./a.out Aborted (core dumped) $ clang-trunk -m32 -O2 -g small.c $ ./a.out Aborted (core dumped) $ -------------------------------- int a, b, c, e, f, g, h; char d; int main () { f = 0; h = d > 0; c = g; e--; a = b <= 0; e && (g = 0); g--; d ^= 1; if (d != 1) __builtin_abort (); return 0; }
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<def> = ...", 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?
(In reply to comment #2) > 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?
Created attachment 15092 [details] 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<DEF> = ... ;; (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 24535 has been marked as a duplicate of this bug. ***
*** Bug 24991 has been marked as a duplicate of this bug. ***
*** Bug 24992 has been marked as a duplicate of this bug. ***
*** Bug 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.