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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: llvm/trunk/lib/CodeGen/MachineInstrBundle.cpp:313
@@ -312,3 +312,3 @@
 
-    bool IsRegOrSuperReg = MOReg == Reg || TRI->isSubRegister(MOReg, Reg);
+    bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg);
     bool IsRegOrOverlapping = MOReg == Reg || TRI->regsOverlap(MOReg, Reg);
----------------
jfb wrote:
> sanjoy wrote:
> > Was this bit intentional?  I think (without understanding the surrounding code) this is contributing to PR25033.
> Yes, this change was intentional: the code used to get confused about sub/super reg before and my patch would tickle the issues (because it uses AL and AH separately. I believe that the previous code had no clue about subregs, and so other code was simply incorrect but that never manifested because of this bug. I think my fix may tickle other bugs :-)
> 
> Michael Hordijk tracked down the problem in the mailing list:
> 
> The IR:
> ```
>         CALL64pcrel32 <ga:@foo>, <regmask>, %RSP<imp-use>, %RSP<imp-def>,
> %EAX<imp-def>
>         %RAX<def> = MOV64rr %R14<kill>
> ```
> So `CALL` defines `EAX`, and we're asking `computeRegisterLiveness` whether or
> not `RAX` is live.  `computeRegisterLiveness` walks backwards and one thing it
> looks for is whether the register is being defined:
> ```
>     if (IsRegOrSuperReg) {
>         PRI.Defines = true;     // Reg or a super-register is defined.
>         if (!MO.isDead())
>         AllDefsDead = false;
>     }
> ```
> 
> And this is what determines if we (`RAX`) is a register or a super-register
> of the register being defined (`lib/CodeGen/MachineInstrBundle.cpp`):
> ```
> 313:    bool IsRegOrSuperReg = MOReg == Reg || TRI->isSubRegister(MOReg,
> Reg);
> ```
> This will return false, as `RAX` is not a sub register of `EAX`.
> 
I'm not familiar with the LLVM backend, so here is what I've *assumed*
about "clobber" vs. "define":

  - a register is clobbered if it is partially written to

  - a register is defined if it is fully written to either by a move
    to that register, or to some super-register that wholly contains
    the said register

If these assumptions are wrong, then you can ignore everything I've
said below. :)

With these assumptions in place, I read the code in `analyzePhysReg` this way:

`IsRegOrSuperReg == true` ==> if `MOReg` is written to (in the
containing MI), then `Reg` is defined.  Specifically, if `MOReg` is
`EAX` and `Reg` is `RAX` then this is `false`, since even if the
instruction is defines `EAX`, it does not *define* `RAX` (though it
may still clobber it).


`IsRegOrOverlapping == true` ==> if `MOReg` is written to, `Reg` is
clobbered (i.e. partially written to).  This should be true if `MOReg`
is `EAX` and `Reg` is `RAX`.



I think the initial problem (before this change, in the IR you showed) was not that
`IsRegOrSuperReg` was `false` when it should have been true, but that
in `computeRegisterLiveness` we have

      if (Analysis.Kills || Analysis.Clobbers)
        // Register killed, so isn't live.
        return LQR_Dead;

I don't think we can return `LQR_Dead` if `Analysis.Clobbers` is true.


Repository:
  rL LLVM

http://reviews.llvm.org/D6629