codegen does not propagate clobbers onto INLINEASM node for flags

[llvmbot]

Bugzilla Link	3701
Resolution	FIXED
Resolved on	Mar 05, 2009 16:38
Version	1.0
OS	FreeBSD
Attachments	test case, generated asm
Reporter	LLVM Bugzilla Contributor
CC	@EdSchouten,@efriedma-quic,@sunfishcode,@pwo

Extended Description

hi.. when compiling the attached test case I am getting attached asm output. here's a snippet of it:

if (pq->curr == xfer) { /* we are currently scheduled for callback */ USB_BUS_UNLOCK(xfer->xroot->bus); return (1); } /* we are not pending */ USB_BUS_UNLOCK(xfer->xroot->bus); return (0);

(in the actual test case this is run through ccc -E so it's expanded)

this is translated to this asm via "ccc -O2 -S -ffreestanding usb.c" (note that the -O2 is important) to this (snippet):

(1) cmpq %rbx, 56(%rax) #APP movl %fs:0,%rax #NO_APP movq 104(%rbx), %rcx movq 296(%rcx), %rcx movl $4, %edx #APP (2) lock ; cmpxchgl %edx,736(%rcx) ; sete %al ; 1: # atomic_cmpset_int

</ASM code>

note that the result of the cmpq at (1) is never used. and this cmpq corresponds to the "if (pq->curr == xfer)" above. thus 1 is always returned.

[EdSchouten]

I suspect this issue may also cause other files in the FreeBSD kernel to miscompile, such as vfs_mount.c.

[llvmbot]

atomic_cmpset_int clobbers the condition codes, so you should be telling the compiler that: "memory", "cc". However that doesn't fix the problem; it may be the optimizers aren't paying attention to "cc" somewhere.

[lattner]

Interesting problem. Codegen doesn't know that all inline asm should be assumed to clobber EFLAGS on X86, so the scheduler is doing the wrong thing. Very subtle!

[lattner]

Actually, strike that. llvm-gcc and clang should implicitly add EFLAGS for x86 for GCC-style inline asm, but the codegen should respect it when on an llvm inline asm.

[efriedma-quic]

llvm-gcc and clang should implicitly add EFLAGS for x86 for GCC-style
inline asm

That's what the "~{flags}" bit means, AFAIK, and clang is properly outputting it.

[lattner]

ok, it is probably a relatively simple bug where we don't turn ~flags into a clobber of the EFLAGS physreg.

[efriedma-quic]

Perhaps it's not working because there's a miscommunication about the name of the EFLAGS register? From X86RegisterInfo.td:
def EFLAGS : Register<"eflags">;

[lattner]

Is anyone working on this?

[llvmbot]

I'll look at it.

[llvmbot]

llvm-gcc x86 marks the following registers clobbered: flags, fpsr, and dirflag. They are being matched by name. That means x86 target has to rename eflags to flags. fpsr is the x87 FPU status word, it's not currently modeled. Does anyone know what "dirflag" is?

[llvmbot]

It's the "direction flag", see CLD and MOVS instructions.

[llvmbot]

Changing eflags to flags fixed part of the problem. It's still screwed up.
: 0x10202a520, LLVM BB @​0x101d0f780, ID#9:
Predecessors according to CFG: 0x10202a280 (#6)
%reg1056 = MOV64rm %reg1024, 1, %reg0, 104, Mem:LD(8,8) [sunkaddr12 + 0]
CMP64mr %reg1056, 1, %reg0, 56, %reg1024, %EFLAGS, Mem:LD(8,8) [0x101d11ea0 + 0]
INLINEASM <es:movl %fs:0,$0>, 10, %reg1054, 14, %EFLAGS<earlyclobber,def>

[llvmbot]

Blah. Once isel lowered the dependency into physical register "flags" dependency, scheduler can no longer tell which def of "flags" the brcond is using. The llvm ir looks rather strange:

    %43 = icmp eq %struct.usb2_xfer* %42, %xfer             ; <i1> [#uses=1]                                                     
    %44 = tail call %struct.thread* asm sideeffect "movl %fs:0,$0", "=r,~{dirflag},~{fpsr},~{flags}"() nounwind             ; <%\

struct.thread*> [#uses=1]
...
...
%50 = tail call i8 asm sideeffect "\09lock ; \09\09\09cmpxchgl $2,$1 ;\09 sete\09$0 ;\09\091:\09\09\09\09# atomic_cmps
et_int", "={ax},=m,r,{ax},m,{memory},{dirflag},{fpsr},{flags}"(i32 %49, i32 4, i32 %45, i32 %49) nounwind ; [
#uses=1]
%51 = icmp eq i8 %50, 0 ; [#uses=2]
br i1 %43, label %52, label %58

Note br is using %43 which is computed by icmp that comes before the inline asm instructions. x86 isel lowering needs to pull the icmp down.

[efriedma-quic]

Blah. Once isel lowered the dependency into physical register "flags"
dependency, scheduler can no longer tell which def of "flags" the brcond is
using.

(This is speculation, since I can't test right now, so feel free to ignore... but it might be useful.)
That doesn't sound right... my guess is that the scheduler isn't handling registers that aren't allowed to be copied properly. x86 isel doesn't seem to be doing anything wrong; besides the fact that EFLAGS is an unusual register, and that it's using "(implicit EFLAGS)", what x86 isel does here is no different that what it does for CL in variable shifts. It might be an interesting experiment to change the copy cost for eflags to something finite, and see if that works correctly.

Note br is using %43 which is computed by icmp that comes before the inline asm
instructions. x86 isel lowering needs to pull the icmp down.

"Before" doesn't have any meaning until we run the scheduler, does it? X86cmp doesn't take a chain operand...

[llvmbot]

Fixed.
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20090302/074602.html