Page MenuHomePhabricator

[X86FixupBWInsts] Fix miscompilation if sibling sub-register is live.

Authored by a.elovikov on Jan 25 2018, 5:57 AM.

Diff Detail

Event Timeline

a.elovikov created this revision.Jan 25 2018, 5:57 AM

Wouldn't TRI->isSuperOrSubRegisterEq() have the same effect as TRI->checkRegistersAlias()? It isn't clear to me why the new function is needed.

Also, a comment here saying more verbosely what this is meant to be looking for would be helpful. I'm thinking something like this:

// If MO is a use of any part of the destination register but is not equal to OrigDestReg
// or one of its subregisters, we cannot use SuperDestReg. For example, if OrigDestReg
// is  %al then an implicit use of %ah, %ax, %eax, or %rax will prevent us from using
// the %eax register.

BTW, thank you very much for this patch!

Indeed, on X86 checkRegistersAlias is just isSuperOrSubRegisterEq (although the former would be the right choice if that was not so :)).
Comment was added - thanks for the exact text!

andrew.w.kaylor accepted this revision.Jan 25 2018, 11:16 AM

Looks good!

This revision is now accepted and ready to land.Jan 25 2018, 11:16 AM
MatzeB added inline comments.Jan 25 2018, 3:06 PM

There is also TargetRegisterInfo::regsOverlap() which should be slightly more efficient (this is implemented in CodeGen because we have access to register units theres, which the MC layer doesn't know about)


Use regsOverlap instead of isSuperOrSubRegisterEq.


Thanks for the pointer. However, the whole file would benefit from this so I'd like to do the change in a subsequent commit. It should not require a review and can be directly committed, right?

This revision was automatically updated to reflect the committed changes.
MatzeB added inline comments.Jan 29 2018, 9:44 AM

Yes a change simplifying .mir like this is so obvious that it can be directly committed (aka post-commit review).