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

[Mitch Bodart via llvm-commits]

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))

  continue;

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.


http://reviews.llvm.org/D20456

Files:
  lib/CodeGen/CriticalAntiDepBreaker.cpp
  test/CodeGen/X86/pr27681.mir

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D20456.58656.patch
Type: text/x-patch
Size: 6038 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160526/f429940a/attachment.bin>