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

[Quentin Colombet via llvm-commits]

Hi Mitch,

I haven’t looked at the patch but given your description, it sounds like using a proper tracking of the liveness (like LivePhysReg) would be the right way to go.
Does that sound reasonable?

Cheers,
-Quentin

> On May 23, 2016, at 4:06 PM, Mitch Bodart <mitch.l.bodart at intel.com> wrote:
> 
> 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
> 
> 
>