[PATCH] D20456: [CodeGen] Fix problem with X86 byte registers in CriticalAntiDepBreaker
Mitch Bodart via llvm-commits
llvm-commits at lists.llvm.org
Thu May 26 11:37:52 PDT 2016
mbodart updated this revision to Diff 58656.
mbodart added a comment.
I uploaded the alternate fix I alluded to, which is all in ScanInstruction.
This retains the KeepReg setting from Sanjay's fix for pr20020, but performs the other liveness updates.
In particular, DefIndices[%CH] gets updated, and prevents %CH from being used as a replacement register.
Matthias, my comments about tied operands/liveness were for its use in ScanInstruction, which predates
Sanjay's use in PrescanInstruction. I also tested a slight variation of this new change to the effect:
bool Keep = KeepRegs.test(Reg);
if (!Keep && MI.isRegTiedToUseOperand(i))
This also resolves the issue with no other known fails. But I cannot say for certain whether this tweak
is appropriate, so I left it out of the change set. Simililary, I cannot say for certain that dropping my
earlier "alias closure" change is appropriate, but for simplicitly of the change set I dropped it.
If we eventually find ourselves needing more tweaks to the antidependence breaker, I would recommend
simply disabling it. For X86 at least, prior to my post scheduling change it was only being used
for a small number of -mcpu cases, so I would be surprised if there's any signficant performance impact.
And fixing these little issues piecemeal is an onion peeling time sink.
The new pr27681.mir test is my first ever .mir test, and the process by which it was created seems fragile.
I started with an llc IR dump, and deleted things until it successfully compiled and still showed the original
bug and the fix. But various tweaks like dropping -mcpu, or trying -mattr, prevented the original bug
from happening. In the end the test works, but I have no idea how coherent/robust the .mir file is.
Additionally, at least one other issue was uncovered. It varies depending on -mcpu/-mattr, but there
are cases where we choose %bh as the free register to replace %bl, and that doesn't actually break the
dependence! It's a perf issue though, not stability, so I don't want to fix it in this change set. But it's
the reason my test checks for %b instead of %bl.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 6038 bytes
Desc: not available
More information about the llvm-commits