[PATCH] D15320: CodeGen: Redo analyzePhysRegs() and computeRegisterLiveness()

[Matthias Braun via llvm-commits]

MatzeB added inline comments.

================
Comment at: include/llvm/CodeGen/MachineInstrBundle.h:173
@@ +172,3 @@
+
+    /// Reg or an overlapping register is defined.
+    bool Defined;
----------------
jfb wrote:
> MatzeB wrote:
> > jfb wrote:
> > > Can you change uses of "overlapping" here and elsewhere? I think they're part of the confusion.
> > Hmm, not sure what a better description would be.
> > What about "Reg or one of its aliases is defined. The definition may only cover parts of the register." and then
> > "Reg or a super-register is defined. The definition covers the full register." below?
> Sub-register?
> 
> And / or ASCII art comment :-)
I meant that the 2nd quote goes to the 2nd variable like this:

```
    /// Reg or one of its aliases is defined. The definition may only cover
    /// parts of the register.
    bool Defined;
    /// Reg or a super-register is defined. The definition covers the full
    /// register.
    bool FullyDefined;

    /// Reg or ont of its aliases is read. The register may only be read
    /// partially.
    bool Read;
    /// Reg or a super-register is read. The full register is read.
    bool FullyRead;
```

I added the 2nd sentence in all these comments so you do not necessarily have to understand what sub-/super-register means here. To me the comments are perfectly clear now. This is not the place to explain concepts like sub- and superregisters with ASCII art IMO (keep in mind that this is a rarely used heuristic for circumstances where you cannot get the RegisterScavenger/LivePhysRegs working).

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4446
@@ +4445,3 @@
+    if (!AXDead) {
+      // FIXME: If computeRegisterLiveness() reported LQR_Unknown and AX is
+      // dead even though we couldn't determine that, then we are reading an
----------------
jfb wrote:
> hfinkel wrote:
> > jfb wrote:
> > > hfinkel wrote:
> > > > This seems like an odd FIXME to keep now that we have a way to detect the situation. Why don't you detect the LQR_Unknown here and abort instead of doing something the verifier will complain about? Then we can have a FIXME that we really should be using some real analysis here instead of a local search.
> > > I'm not sure I understand what you're suggesting, maybe the new FIXME isn't clear enough: you're right that a real analysis would fix the issue, but if the current one says unknown then we have to save/restore AX otherwise we may be clobbering live state. It's better to complain at verification time rather than to clobber state!
> > Indeed, thanks for clarifying. You're right, we can't do better than this for now.
> OK, it would be good to clarify the FIXME then, if Hal didn't get it then it's not clear enough :)
Again I am not sure where the confusion comes from (obviously I try to write comments that make sense to myself). I am guessing the thing here is the MI representation (or rather the MachineVerifier) requires you to add the "undef" flag when reading from dead registers. We do not (and should not IMO) have a way to express a situation in which we do not know whether a register is dead or not.

Anyway I'll change it to this:
```
      // FIXME: If computeRegisterLiveness() reported LQR_Unknown then AX may
      // actually be dead. This is not a problem for correctness as we are just
      // (unnecessarily) saving+restoring a dead register. However the
      // MachineVerifier expects operands that read from dead registers
      // to be marked with the "undef" flag.
```

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4454
@@ -4453,3 @@
-    //        Once fixed, also update cmpxchg-clobber-flags.ll and
-    //        peephole-na-phys-copy-folding.ll.
-
----------------
jfb wrote:
> MatzeB wrote:
> > jfb wrote:
> > > Could you also fix peephole-na-phys-copy-folding.ll? Or does it trigger the verifier as the new FIXME says? We could pass a bigger `Neighborhood` value to `computeRegisterLiveness` if that's the case.
> > Yes peephole-na-phys-copy-folding is affected by the FIXME I added (I'll add the name of the test to the comment). This is still an unsolved issue before and after the changes. Increasing the Neighborhood does not help as we need an "infinite" neighborhood to cover all cases. To get a precise liveness query we would need a LivePhysRegs (or RegisterScavenger) instance which ideally is kept up to date while we walk backwards over the basic blocks unfortunately ExpandPostRAPseudos has no easy way to maintain state and walks forward at the moment, so fixing this properly is work for another patch.
> OK, keeping the comment here would be nice. The test file also has a FIXME which you seem to say can't be fixed while addressing the issue it mentions? Maybe it would be worth pulling out the tests that fail validation into their own test that don't have validation (so the other ones can be validated independently)?
Strange, I looked at peephole-na-phys-copy-folding again and it works with -verify-machineinstrs (there are not emergency push ax generated anymore). I must have tested an intermediate version. So I'll just remove the FIXME and add -verify-machineinstrs to peephole-na-phys-copy-folding.ll


Repository:
  rL LLVM

http://reviews.llvm.org/D15320