<div dir="ltr"><a class="GWVZpf gW" id="IloFPc-0" href="mailto:tstellar@redhat.com" tabindex="-1">+tstellar@redhat.com</a> - We almost certainly want to pull this into whatever the next release is. =[<br><div><br></div><div><a class="GWVZpf gW" id="IloFPc-1" href="mailto:craig.topper@gmail.com" tabindex="-1">+Craig Topper</a> for reference....<br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 31, 2018 at 8:02 PM Chandler Carruth via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: chandlerc<br>
Date: Tue Jul 31 20:01:58 2018<br>
New Revision: 338481<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=338481&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=338481&view=rev</a><br>
Log:<br>
[x86] Fix a really subtle miscompile due to a somewhat glaring bug in<br>
EFLAGS copy lowering.<br>
<br>
If you have a branch of LLVM, you may want to cherrypick this. It is<br>
extremely unlikely to hit this case empirically, but it will likely<br>
manifest as an "impossible" branch being taken somewhere, and will be<br>
... very hard to debug.<br>
<br>
Hitting this requires complex conditions living across complex control<br>
flow combined with some interesting memory (non-stack) initialized with<br>
the results of a comparison. Also, because you have to arrange for an<br>
EFLAGS copy to be in *just* the right place, almost anything you do to<br>
the code will hide the bug. I was unable to reduce anything remotely<br>
resembling a "good" test case from the place where I hit it, and so<br>
instead I have constructed synthetic MIR testing that directly exercises<br>
the bug in question (as well as the good behavior for completeness).<br>
<br>
The issue is that we would mistakenly assume any SETcc with a valid<br>
condition and an initial operand that was a register and a virtual<br>
register at that to be a register *defining* SETcc...<br>
<br>
It isn't though....<br>
<br>
This would in turn cause us to test some other bizarre register,<br>
typically the base pointer of some memory. Now, testing this register<br>
and using that to branch on doesn't make any sense. It even fails the<br>
machine verifier (if you are running it) due to the wrong register<br>
class. But it will make it through LLVM, assemble, and it *looks*<br>
fine... But wow do you get a very unsual and surprising branch taken in<br>
your actual code.<br>
<br>
The fix is to actually check what kind of SETcc instruction we're<br>
dealing with. Because there are a bunch of them, I just test the<br>
may-store bit in the instruction. I've also added an assert for sanity<br>
that ensure we are, in fact, *defining* the register operand. =D<br>
<br>
Modified:<br>
llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp<br>
llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir<br>
<br>
Modified: llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp?rev=338481&r1=338480&r2=338481&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp?rev=338481&r1=338480&r2=338481&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp (original)<br>
+++ llvm/trunk/lib/Target/X86/X86FlagsCopyLowering.cpp Tue Jul 31 20:01:58 2018<br>
@@ -730,9 +730,12 @@ CondRegArray X86FlagsCopyLoweringPass::c<br>
for (MachineInstr &MI :<br>
llvm::reverse(llvm::make_range(MBB.begin(), TestPos))) {<br>
X86::CondCode Cond = X86::getCondFromSETOpc(MI.getOpcode());<br>
- if (Cond != X86::COND_INVALID && MI.getOperand(0).isReg() &&<br>
- TRI->isVirtualRegister(MI.getOperand(0).getReg()))<br>
+ if (Cond != X86::COND_INVALID && !MI.mayStore() && MI.getOperand(0).isReg() &&<br>
+ TRI->isVirtualRegister(MI.getOperand(0).getReg())) {<br>
+ assert(MI.getOperand(0).isDef() &&<br>
+ "A non-storing SETcc should always define a register!");<br>
CondRegs[Cond] = MI.getOperand(0).getReg();<br>
+ }<br>
<br>
// Stop scanning when we see the first definition of the EFLAGS as prior to<br>
// this we would potentially capture the wrong flag state.<br>
<br>
Modified: llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir?rev=338481&r1=338480&r2=338481&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir?rev=338481&r1=338480&r2=338481&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir (original)<br>
+++ llvm/trunk/test/CodeGen/X86/flags-copy-lowering.mir Tue Jul 31 20:01:58 2018<br>
@@ -90,6 +90,18 @@<br>
call void @foo()<br>
ret i64 0<br>
}<br>
+<br>
+ define i32 @test_existing_setcc(i64 %a, i64 %b) {<br>
+ entry:<br>
+ call void @foo()<br>
+ ret i32 0<br>
+ }<br>
+<br>
+ define i32 @test_existing_setcc_memory(i64 %a, i64 %b) {<br>
+ entry:<br>
+ call void @foo()<br>
+ ret i32 0<br>
+ }<br>
...<br>
---<br>
name: test_branch<br>
@@ -936,3 +948,110 @@ body: |<br>
; CHECK: %8:gr64 = CMOVE64rr %0, %1, implicit killed $eflags<br>
<br>
...<br>
+---<br>
+name: test_existing_setcc<br>
+# CHECK-LABEL: name: test_existing_setcc<br>
+liveins:<br>
+ - { reg: '$rdi', virtual-reg: '%0' }<br>
+ - { reg: '$rsi', virtual-reg: '%1' }<br>
+body: |<br>
+ bb.0:<br>
+ successors: %bb.1, %bb.2, %bb.3<br>
+ liveins: $rdi, $rsi<br>
+<br>
+ %0:gr64 = COPY $rdi<br>
+ %1:gr64 = COPY $rsi<br>
+ CMP64rr %0, %1, implicit-def $eflags<br>
+ %2:gr8 = SETAr implicit $eflags<br>
+ %3:gr8 = SETAEr implicit $eflags<br>
+ %4:gr64 = COPY $eflags<br>
+ ; CHECK: CMP64rr %0, %1, implicit-def $eflags<br>
+ ; CHECK-NEXT: %[[A_REG:[^:]*]]:gr8 = SETAr implicit $eflags<br>
+ ; CHECK-NEXT: %[[AE_REG:[^:]*]]:gr8 = SETAEr implicit $eflags<br>
+ ; CHECK-NOT: COPY{{( killed)?}} $eflags<br>
+<br>
+ ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp<br>
+ CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, implicit-def $eax<br>
+ ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp<br>
+<br>
+ $eflags = COPY %4<br>
+ JA_1 %bb.1, implicit $eflags<br>
+ JB_1 %bb.2, implicit $eflags<br>
+ JMP_1 %bb.3<br>
+ ; CHECK-NOT: $eflags =<br>
+ ;<br>
+ ; CHECK: TEST8rr %[[A_REG]], %[[A_REG]], implicit-def $eflags<br>
+ ; CHECK-NEXT: JNE_1 %bb.1, implicit killed $eflags<br>
+ ; CHECK-SAME: {{$[[:space:]]}}<br>
+ ; CHECK-NEXT: bb.4:<br>
+ ; CHECK-NEXT: successors: {{.*$}}<br>
+ ; CHECK-SAME: {{$[[:space:]]}}<br>
+ ; CHECK-NEXT: TEST8rr %[[AE_REG]], %[[AE_REG]], implicit-def $eflags<br>
+ ; CHECK-NEXT: JE_1 %bb.2, implicit killed $eflags<br>
+ ; CHECK-NEXT: JMP_1 %bb.3<br>
+<br>
+ bb.1:<br>
+ %5:gr32 = MOV32ri64 42<br>
+ $eax = COPY %5<br>
+ RET 0, $eax<br>
+<br>
+ bb.2:<br>
+ %6:gr32 = MOV32ri64 43<br>
+ $eax = COPY %6<br>
+ RET 0, $eax<br>
+<br>
+ bb.3:<br>
+ %7:gr32 = MOV32r0 implicit-def dead $eflags<br>
+ $eax = COPY %7<br>
+ RET 0, $eax<br>
+<br>
+...<br>
+---<br>
+name: test_existing_setcc_memory<br>
+# CHECK-LABEL: name: test_existing_setcc_memory<br>
+liveins:<br>
+ - { reg: '$rdi', virtual-reg: '%0' }<br>
+ - { reg: '$rsi', virtual-reg: '%1' }<br>
+body: |<br>
+ bb.0:<br>
+ successors: %bb.1, %bb.2<br>
+ liveins: $rdi, $rsi<br>
+<br>
+ %0:gr64 = COPY $rdi<br>
+ %1:gr64 = COPY $rsi<br>
+ CMP64rr %0, %1, implicit-def $eflags<br>
+ SETEm %0, 1, $noreg, -16, $noreg, implicit $eflags<br>
+ %2:gr64 = COPY $eflags<br>
+ ; CHECK: CMP64rr %0, %1, implicit-def $eflags<br>
+ ; We cannot reuse this SETE because it stores the flag directly to memory,<br>
+ ; so we have two SETEs here. FIXME: It'd be great if something could fold<br>
+ ; these automatically. If not, maybe we want to unfold SETcc instructions<br>
+ ; writing to memory so we can reuse them.<br>
+ ; CHECK-NEXT: SETEm {{.*}} implicit $eflags<br>
+ ; CHECK-NEXT: %[[E_REG:[^:]*]]:gr8 = SETEr implicit $eflags<br>
+ ; CHECK-NOT: COPY{{( killed)?}} $eflags<br>
+<br>
+ ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp<br>
+ CALL64pcrel32 @foo, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp, implicit-def $eax<br>
+ ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp<br>
+<br>
+ $eflags = COPY %2<br>
+ JE_1 %bb.1, implicit $eflags<br>
+ JMP_1 %bb.2<br>
+ ; CHECK-NOT: $eflags =<br>
+ ;<br>
+ ; CHECK: TEST8rr %[[E_REG]], %[[E_REG]], implicit-def $eflags<br>
+ ; CHECK-NEXT: JNE_1 %bb.1, implicit killed $eflags<br>
+ ; CHECK-NEXT: JMP_1 %bb.2<br>
+<br>
+ bb.1:<br>
+ %3:gr32 = MOV32ri64 42<br>
+ $eax = COPY %3<br>
+ RET 0, $eax<br>
+<br>
+ bb.2:<br>
+ %4:gr32 = MOV32ri64 43<br>
+ $eax = COPY %4<br>
+ RET 0, $eax<br>
+<br>
+...<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>