[PATCH] D20456: [CodeGen] Fix problem with X86 byte registers in CriticalAntiDepBreaker
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Mon May 23 17:13:30 PDT 2016
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?
> 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.
More information about the llvm-commits