[PATCH] x86 NaCl: Emit LAHF/SAHF instead of PUSHF/POPF

[JF Bastien]

Following up with @t.p.northover's earlier comments:

In http://reviews.llvm.org/D6629#101719, @t.p.northover wrote:

> > > - What happens for "EAX = COPY EFLAGS"? (It's not good).
>
> > 
>
> > 
>
> > You mean if the user actually wants to copy `EFLAGS` for system code? Indeed that would be broken.
>
>
> No, I mean if LLVM happened to decide to spill EFLAGS to EAX (allocate
>  vreg4 to EAX in your example). The final pop would clobber the value
>  we'd just carefully constructed.
>
> > > - I'm not sure what's available here, but there are ways to check liveness too, which would allow the push/pop to be skipped entirely if AX is dead.
>
> > 
>
> > 
>
> > I'm not very familiar with this code, guidance appreciated.
>
>
> I meant MachineBasicBlock::computeRegisterLiveness. If it tells us AX
>  might be live, save it, otherwise skip it.


Done in my latest update, though it looks like this is exposing a bug. I think cmpxchg is correctly annotated as def+kill of EAX, but the liveness analysis seems to think it's dead:

  BB#0: derived from LLVM BB %0
      Live Ins: %EBX %ESI
  	PUSH32r %EBX<kill>, %ESP<imp-def>, %ESP<imp-use>; flags: FrameSetup
  	CFI_INSTRUCTION <call frame instruction>
  	PUSH32r %ESI<kill>, %ESP<imp-def>, %ESP<imp-use>; flags: FrameSetup
  	CFI_INSTRUCTION <call frame instruction>
  	CFI_INSTRUCTION <call frame instruction>
  	CFI_INSTRUCTION <call frame instruction>
  	%EAX<def> = MOV32rm %ESP, 1, %noreg, 16, %noreg; mem:LD4[FixedStack-2]
  	%EDX<def> = MOV32rm %ESP, 1, %noreg, 20, %noreg; mem:LD4[FixedStack-3]
  	%EBX<def> = MOV32rm %ESP, 1, %noreg, 24, %noreg; mem:LD4[FixedStack-4]
  	%ECX<def> = MOV32rm %ESP, 1, %noreg, 28, %noreg; mem:LD4[FixedStack-5]
  	%ESI<def> = MOV32rm %ESP, 1, %noreg, 12, %noreg; mem:LD4[FixedStack-1]
  	LCMPXCHG8B %ESI<kill>, 1, %noreg, 0, %noreg, %EAX<imp-def,dead>, %EDX<imp-def,dead>, %EFLAGS<imp-def>, %EAX<imp-use>, %EBX<imp-use>, %ECX<imp-use>, %EDX<imp-use>; mem:Volatile LDST8[%foo]
  	%AL<def> = SETOr %EFLAGS<imp-use>
  	LAHF %AH<imp-def>, %EFLAGS<imp-use>
  	%ESI<def> = MOV32rr %EAX
  	CALLpcrel32 <ga:@bar>, <regmask>, %ESP<imp-use>, %ESP<imp-def>, %EAX<imp-def,dead>
  	%EAX<def> = MOV32rr %ESI<kill>
  	%AL<def,tied1> = ADD8ri %AL<tied0>, 127, %EFLAGS<imp-def>
  	SAHF %EFLAGS<imp-def>, %AH<imp-use>
  	JNE_4 <BB#3>, %EFLAGS<imp-use>
      Successors according to CFG: BB#1(16) BB#3(16)



> > > - We probably want to be more careful with the flags on the instructions (at the least LAHF & SAHF should be marked with EFLAGS I think, probably we should get the kill states right too).

> 

> > 

> 

> > 

> 

> > I think that's already the case, unless I misunderstand what you want. See  `test_intervening_call`  after post-RA expansion (when `LAHF`/`SAHF` get added):

> 

> 

> Oh good, it obviously comes from the MCInstrDesc somehow. The flags

>  aren't quite correct though:


I updated push to kill AX. I'm not sure which other maintenance should be done on the liveness state.

> > > - With all this complexity, it might be time for a helper function.

> 

> > 

> 

> > 

> 

> > You mean: I should pull the `EFLAGS` handling out of `X86InstrInfo::copyPhysReg`? I can do that, I just want to make sure that's what you're suggesting.

> 

> 

> Yep, that's what I meant.


Haven't done that yet, just to keep the diff easier to follow.


http://reviews.llvm.org/D6629

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/