This is an archive of the discontinued LLVM Phabricator instance.

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

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
lib/Target/X86/X86FixupBWInsts.cpp
256

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
lib/Target/X86/X86FixupBWInsts.cpp
256

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)

test/CodeGen/X86/fixup-bw-inst.mir
204–246

Use regsOverlap instead of isSuperOrSubRegisterEq.

test/CodeGen/X86/fixup-bw-inst.mir
204–246

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
test/CodeGen/X86/fixup-bw-inst.mir
204–246

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