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

[Tim Northover]

Hi,

On 15 December 2014 at 15:41, JF Bastien <jfb at chromium.org> 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.

> That looks like something PeepholeOptimizer.cpp should do (like it does with `X86InstrInfo::optimizeCompareInstr` and similar),

I don't follow here, I'm afraid

> Do we care to do this, though? This code shouldn't get emitted often, so is it worth optimizing/testing/maintaining?

I think so.

>
>> - 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:

>         PUSH64r %RAX, %RSP<imp-def>, %RSP<imp-use>
>         %RAX<def> = SETOr %EFLAGS<imp-use>
>         LAHF %AH<imp-def>, %EFLAGS<imp-use>
>         %RBX<def> = MOV64rr %RAX
>         %RAX<def> = POP64r %RSP<imp-def>, %RSP<imp-use>

Line 1 should kill RAX; line 3 might need to kill EFLAGS (depending on
the incoming COPY) and I think needs to mention RAX as well (recent
modifications to support sub-register liveness may have change that, I
wasn't paying much attention; check with more natural sequences what
should be done); line 4 should kill RAX.

>         PUSH64r %RAX, %RSP<imp-def>, %RSP<imp-use>
>         %RAX<def> = MOV64rr %RBX<kill>
>         %AL<def,tied1> = ADD8ri %AL<tied0>, 127, %EFLAGS<imp-def>
>         SAHF %EFLAGS<imp-def>, %AH<imp-use>
>         %RAX<def> = POP64r %RSP<imp-def>, %RSP<imp-use>

This has similar issues.

>> - 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.

Cheers.

Tim.