[PATCH] D20456: [CodeGen] Fix problem with X86 byte registers in CriticalAntiDepBreaker

[Mitch Bodart via llvm-commits]

mbodart added a comment.

Sanjay, I tried backing out your fix for pr20020.ll, but the test passes either way.  So something else changed along the way.
This is a great argument for using a .mir test, and I'll pursue that.

I'm testing an alternative "fix" isolated to ScanInstruction.  Instead of skipping the state updates when we see Reg in KeepRegs, I do all of the state updates except for removing Reg from KeepRegs.  But I also heed the immediate continue, with no state updates, in the tied operand case.  In particular this will reflect in DefIndices and Classes that %CH is modified by the %ECX def, and that's enough to prevent using %CH in findSuitableFreeRegister.

I did some more digging re the tied operand check, and I think the idea there is that a tied operand will still be live into the instruction, so apparently the author thought this meant the def need not be treated as a live range killer.  Mucking with that assumption is not something I want to delve into with this particular change.

Matthias, this isn't just being enabled now.  The X86 post scheduler had been enabled for atom, bonnell, btver2 and slm.
I agree with your general concern re the stability of the antidepence breaker, simply from paranoia after staring at the code.
But a complete overhaul is a longer term project.

For the short term I agree with Quentin that a small, safe, patch is reasonable.
But per one of Sanjay's bugzilla comments, it may also be reasonable to disable the antidepence breaker for X86.
We'd have to run some performance tests to verify that.


http://reviews.llvm.org/D20456