[PATCH] D20456: [CodeGen] Fix problem with X86 byte registers in CriticalAntiDepBreaker
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Wed May 25 13:41:24 PDT 2016
MatzeB added a comment.
In http://reviews.llvm.org/D20456#439675, @mbodart wrote:
> 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.
My understanding is that some things cannot be renamed legally and KeepRegs collects those:
For calls the ABI forces some values to some register, and for two-address code (the tied-operand case) we cannot simply rename the input because it has to match the output. That much of the code seems fine to me.
> 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.
Yes having a fix for the issue at hand is perfectly fine, I am not trying to talk you into rewriting the pass. I was only objecting to the patch as it stands right now because adding the super/sub register iterator this way for a tied operand seems to fix the issue by accident not by design.
More information about the llvm-commits