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

[Mitch Bodart via llvm-commits]

mbodart added a comment.

I was also not familiar with this code, until diving in to fix pr27681.

KeepRegs was introduced in r83198 of PostRAScheduler.cpp back in 2009,
and the tied operand checks existed before then.  In r212275 of
CriticalAntiDepBreaker.cpp (July 2014), and possibly other change
sets along the way, the use of KeepRegs and tied operands was tweaked
to fix pr20020.

From what I can surmise, the original intent of KeepRegs was to mark
registers as "hands off", when looking for a candidate anti-dependent
register def.  So this change is muddying the water a little bit,
as I'm now also using it to say "hands off" when choosing a replacement
register.  However, I did not see any other obvious solution.
Setting Classes[Reg] to -1 is another way to say "hands off"
for the purposes of choosing a replacement reg, but it's not clear
how that can be made to work in the current structure for this case.

There is an assumption in ScanInstruction that if a def'd Reg is in KeepRegs,
or is tied to a use, then we do not need to update the liveness info
(Def/KillIndices, KeepRegs, Classes, RegRefs) for it or any of its subregs,
nor set Classes to -1 for any of its superregs.

In the case of pr27681, this means those datastructures are not updated
for %CH when we see a def of %ECX, and ultimately %CH is incorrectly treated
as a suitable replacement register in findSuitableFreeRegister.

I don't know why tied operands are treated the way they are, but the bug
is not a direct consequence of tied operands.  Rather, it is in the assumptions
being made about registers present in the KeepRegs set, and implicitly
their subregs.

As to the iterator question, yes it is quadratic, and no, MCRegAliasIterator
does not suffice.  If you know of an iterator that would fill the need here,
great.  But I couldn't find one.  I'm not worried about the quadratic nature,
as the sizes here are very small.


http://reviews.llvm.org/D20456