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

[Matthias Braun via llvm-commits]

MatzeB added a comment.

In http://reviews.llvm.org/D20456#438492, @mbodart wrote:

> I wasn't familiar with .mir files, so thanks for pointing them out!
>  Unfortunately creating one directly from the original pr27681.ll file yields
>  a .mir file which incurs various parse and semantic errors when run through
>  llc, so it may take some effort to get a clean .mir file.  Perhaps someone
>  with more experience than I can whip one out quickly?


That is unfortunate. .mir files are still a new thing and have many rough edges, nonetheless they are often the only way to produce a sane and stable testcase. It would be nice if you could file a bug with instructions on how to reproduce the problem.

> Wrt respecting sub/super regs, there are several areas in this optimization

>  that look suspect to me.  But that doesn't necessarily mean they are incorrect.

>  Maybe they are, maybe they aren't but it requires further study.

>  As just one example, right off the bat in StartBlock, %CL is found to be

>  live out.  So %CL, and rightfully its superregs %CX, %ECX and %RCX are

>  also set to a live state, but %CH is not.  It certainly appears odd that

>  %CH's superregs are live while %CH is not, but it's technically accurate.

>  And it illustrates that properties of a superreg do not necessarily apply

>  to its subregs.

> 

> > For a proper fix we should have findSuitableFreeRegister() report false for %ch.

> 

> 

> Wholeheartedly agree and that's what I'm trying to achieve.

> 

> The isNewRegClobberedByRefs suggestion is unfortunately insufficient.

>  The refs in this case are the references to the antidependent register %BL,

>  which does not include the instruction that defines %ECX.

> 

> I'm not familiar with LivePhysReg, so I don't know how difficult it would

>  be to apply here (well, both because of that unfamiliarity, and because I've

>  found it very difficult to associate a definitive meaning to the liveness

>  data structures maintained here).  Fundamentally I think the shortcuts being

>  taken for tied operands, and registers already present in the KeepRegs set,

>  are a key piece of the problem.  So even if LivePhysReg were employed,

>  those shortcuts could still lead to issues.


While I would strongly recommend LivePhysRegs for newly written code, I fear that it means rewriting (bigger?) parts of the pass here. Given how confusing the current code is we have to be careful not to introduce new bugs because we do not understand the current code...

> We need to recognize that the %ECX def clobbers %CH.  Normally this appears

>  to be achieved in ScanInstruction by updating DefIndices[%CH] and Classes[%CH].

>  But that processing is skipped because of %ECX's presence in KeepRegs,

>  which in turn was caused by %CL having a tied instance.


Ah that reasoning makes sense. What happens if you remove that KeepRegs check there? On first sight this just seems like a shortcut to me that we can do without...

> I'll spend a little more time looking at those shortcuts to see if

>  a straightforward solution comes to mind, using DefIndices and Classes.

>  But if a major revamp comes into play, I likely won't have the time

>  to tackle it.



http://reviews.llvm.org/D20456