This is an archive of the discontinued LLVM Phabricator instance.

[X86] Defer the creation of LCMPXCHG16B_SAVE_RBX until finalize-isel
ClosedPublic

Authored by craig.topper on Oct 4 2020, 9:17 PM.

Details

Summary

We need to use LCMPXCHG16B_SAVE_RBX if RBX/EBX is being used as
the frame pointer. We previously checked for this during type
legalization, but that's too early to know for sure if the base
pointer is needed.

This patch adds a new pseudo instruction to emit from isel that
uses a virtual register for the RBX input. Then we use the custom
inserter hook to emit LCMPXCHG16B if RBX isn't needed as a base
pointer or LCMPXCHG16B_SAVE_RBX if it is.

The test case is taken from the bugzilla with minor cleanup, but
it's still pretty large.

This also removes LCMPXCHG8B_SAVE_EBX which was not used.

Fixes PR42064.

Diff Detail

Event Timeline

craig.topper created this revision.Oct 4 2020, 9:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2020, 9:17 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript
craig.topper requested review of this revision.Oct 4 2020, 9:17 PM
craig.topper edited the summary of this revision. (Show Details)Oct 4 2020, 9:17 PM

Do we gain anything by getting rid of LCMPXCHG8B_SAVE_EBX first?

Do we gain anything by getting rid of LCMPXCHG8B_SAVE_EBX first?

Maybe a little bit. I'll split it out.

rnk added a comment.Oct 5 2020, 1:41 PM

Can we go even later? See https://reviews.llvm.org/rGcda6b0924257f162cc9299dae2d4bb134fac5d38, which goes all the way to X86ExpandPseudo.

llvm/test/CodeGen/X86/pr42064.ll
46

Can this be reduced a little? I'm guessing something here, maybe these stack memsets, are increasing the alignment of a stack object during ISel, which causes us to use a base pointer. You probably only need one funclet and one of whatever increases the alignment to trigger the bug.

In D88808#2312741, @rnk wrote:

Can we go even later? See https://reviews.llvm.org/rGcda6b0924257f162cc9299dae2d4bb134fac5d38, which goes all the way to X86ExpandPseudo.

The mwaitx patch is making the decision about frame pointer being used in the custom inserter same as this patch.

llvm/test/CodeGen/X86/pr42064.ll
46

Probably I didn't look very closely.

Rebase on top of the LCMPXCHG8B_SAVE_EBX removal. Unfortunately it looks like a missed a couple spots in that patch.

llvm/lib/Target/X86/X86ExpandPseudo.cpp
341

I missed these comments in the other patch.

llvm/lib/Target/X86/X86ISelLowering.h
759

I missed this in the other patch. Hopefully its not too distracting

-Reduce test case

LGTM but I'd like to see if someone has objections.

llvm/lib/Target/X86/X86ISelLowering.cpp
33784

Why don't we use the same form as line 33780?

llvm/lib/Target/X86/X86InstrCompiler.td
891

Maybe we can leave it "" since we don't use its assmebly.

Address comments

pengfei accepted this revision.Oct 6 2020, 6:37 PM

LGTM.

This revision is now accepted and ready to land.Oct 6 2020, 6:37 PM
dstenb added a subscriber: dstenb.Oct 9 2020, 2:43 AM

This seems to have broken the expensive-checks build bots:

http://lab.llvm.org:8011/#/builders/16/builds/33/steps/6/logs/FAIL__LLVM__pr42064_ll

*** Bad machine code: Using an undefined physical register ***
- function:    foo
- basic block: %bb.1 bb7 (0x65cf848)
- instruction: LCMPXCHG16B killed renamable $r8, 1, $noreg, 0, $noreg, implicit-def $rax, implicit-def $rdx, implicit-def $eflags, implicit $rax, implicit $rbx, implicit $rcx, implicit $rdx
- operand 10:   implicit $rcx
LLVM ERROR: Found 1 machine code errors.
dmajor added a subscriber: dmajor.Oct 9 2020, 10:47 AM