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

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: include/llvm/CodeGen/MachineInstrBundle.h:173
@@ +172,3 @@
+
+    /// Reg or an overlapping register is defined.
+    bool Defined;
----------------
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 :-)

================
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.
-
----------------
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)?


Repository:
  rL LLVM

http://reviews.llvm.org/D15320