This is an archive of the discontinued LLVM Phabricator instance.

[X86] Detect if EFLAGs is live across XBEGIN pseudo instruction. Add it as livein to the basic blocks created when expanding the pseudo
ClosedPublic

Authored by craig.topper on Jul 23 2020, 5:11 PM.

Details

Summary

XBEGIN causes several based blocks to be inserted. If flags are live across it we need to make eflags live in the new basic blocks to avoid machine verifier errors.

Is there a better way to do this than just scanning the rest of the basic block of uses or defs?

Fixes PR46827

Diff Detail

Event Timeline

craig.topper created this revision.Jul 23 2020, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2020, 5:11 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

@craig.topper Thanks for fixing this!

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

So this situation can happen *only* for $EFLAGS?

craig.topper marked an inline comment as done.Jul 24 2020, 10:42 AM
craig.topper added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
31005

I don't know for sure. I copied the code from EmitLoweredSelect where expanding CMOV pseudos has the same issue.

djtodoro added inline comments.Jul 24 2020, 10:58 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
31005

Can we make a static function and reuse the interface? I guess we can face this in some other pseudo instructions expanding as well.

Share code with CMOV pseudo expansion. Also check for liveins of the successor BBs which was done by cmov.

Refactor code to just return false when we find a def. Then we don't have to check explicitly for end of the block. If we fall out of the loop then we must have reached the end of the block.

ivanbaev requested changes to this revision.EditedJul 24 2020, 9:38 PM
ivanbaev added a subscriber: ivanbaev.

Hi Craig, thank you for working on this bug.

  1. * IR Dump Before Finalize ISel and expand pseudo-instructions *:
  2. Machine code for function wobble.12: IsSSA, TracksLiveness

bb.0.bb107:

successors: %bb.1(0x40000000), %bb.2(0x40000000); %bb.1(50.00%), %bb.2(50.00%)

%0:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0, align 16)
%1:gr32 = SUB32ri8 %0:gr32(tied-def 0), 1, implicit-def $eflags
%2:gr32 = XBEGIN
JCC_1 %bb.2, 5, implicit $eflags
JMP_1 %bb.1

...


Given the IR above for the test, an alternative liveness check for eflags is to iterate backward.
Look for a def of eflag, and if we reach the start of the basic block: check BB->isLiveIn(X86::EFLAGS)) and avoid iterating over successors of BB as in the proposed fix.

As optimization add EFLAGS only to sinkMBB
sinkMBB->addLiveIn(X86::EFLAGS);

The proposed fix is good too. It just seems to me that connecting a def to its uses is more natural.

This revision now requires changes to proceed.Jul 24 2020, 9:38 PM

Hi Craig, thank you for working on this bug.

  1. * IR Dump Before Finalize ISel and expand pseudo-instructions *:
  2. Machine code for function wobble.12: IsSSA, TracksLiveness

bb.0.bb107:

successors: %bb.1(0x40000000), %bb.2(0x40000000); %bb.1(50.00%), %bb.2(50.00%)

%0:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0, align 16)
%1:gr32 = SUB32ri8 %0:gr32(tied-def 0), 1, implicit-def $eflags
%2:gr32 = XBEGIN
JCC_1 %bb.2, 5, implicit $eflags
JMP_1 %bb.1

...


Given the IR above for the test, an alternative liveness check for eflags is to iterate backward.
Look for a def of eflag, and if we reach the start of the basic block: check BB->isLiveIn(X86::EFLAGS)) and avoid iterating over successors of BB as in the proposed fix.

We'd have to also use the kill flag for any users we find before we find the def? We shouldn't make it livein if it was already killed.

As optimization add EFLAGS only to sinkMBB
sinkMBB->addLiveIn(X86::EFLAGS);

Can we have it live in in sinkMBB without it being livein in the predecessors in sinkMBB?

The proposed fix is good too. It just seems to me that connecting a def to its uses is more natural.

Yes, check for mi.killsRegister(X86::EFLAGS)) before a def (in backward order).
The instructions in mainMBB and fallBB during xbegin() expansion do not deal with EFLAGS. If a subsequent pass makes changes in mainMBB and fallBB that might impact the liveness of EFLAGS, that pass should make proper updates. Having

mainMBB->addLiveIn(X86::EFLAGS);
fallMBB->addLiveIn(X86::EFLAGS);

is good too.

Yes, check for mi.killsRegister(X86::EFLAGS)) before a def (in backward order).
The instructions in mainMBB and fallBB during xbegin() expansion do not deal with EFLAGS. If a subsequent pass makes changes in mainMBB and fallBB that might impact the liveness of EFLAGS, that pass should make proper updates. Having

mainMBB->addLiveIn(X86::EFLAGS);
fallMBB->addLiveIn(X86::EFLAGS);

is good too.

Removing the addLiveIn from mainMBB/fallMBB gives

*** Bad machine code: Live in register not found to be live out from predecessor. ***
- function:    wobble.12
- basic block: %bb.5 bb107 (0x7f8863865bc0)
EFLAGS not found to be live out from %bb.3

*** Bad machine code: Live in register not found to be live out from predecessor. ***
- function:    wobble.12
- basic block: %bb.5 bb107 (0x7f8863865bc0)
EFLAGS not found to be live out from %bb.4
LLVM ERROR: Found 2 machine code errors.

Thank you for checking on mainMBB and fallBB. Which direction: forward or backward - do you prefer for isEFLAGSLiveAfter()?

Thank you for checking on mainMBB and fallBB. Which direction: forward or backward - do you prefer for isEFLAGSLiveAfter()?

I think I'd prefer to keep the forward direction. The code is shared with cmov handling which is also fixing incorrect kill flags on the cmov pseudo instruction.

I don't have strong preference for bwd direction. Let's get this fix in. Are all tests and sanities passing?

I don't have strong preference for bwd direction. Let's get this fix in. Are all tests and sanities passing?

ninja check-llvm passes

@craig.topper Thanks for addressing the comments, lgtm now.

LGTM. How to change NeedRevision flag?

ivanbaev accepted this revision.Jul 27 2020, 7:44 AM
This revision is now accepted and ready to land.Jul 27 2020, 7:44 AM