Page MenuHomePhabricator

[X86] Remove EFLAGS from live-in lists of successor MBBs in X86FlagsCopyLowering
ClosedPublic

Authored by jonpa on Dec 11 2019, 1:08 PM.

Details

Summary

This is an attempt to handle the problem discussed in https://reviews.llvm.org/D68267.

I haven't touched this is sometime. The thing I would worry about is whether *all* of the edges into the successor have been rewritten such that EFLAGS no longer needs to live-in. I don't recall whether the code ensures that is the case in a clear point, but if it does, that is the place to update the live-in list. (I suspect it does somewhere, and it is just finding it in order to do the update correctly... my guess is that it'll be at the end because we're operating edge-wise and may not have finished rewriting edges into the successor... but that's just a guess.)

As far as I can understand, X86FlagsCopyLoweringPass handles one COPY to X86::EFLAGS at a time by finding all its users. The users are found in the current MBB and all the successors (recursively) that has EFLAGS live-in. Each MBB is only visited once and the users are rewritten on the fly, except the jump instructions which are done after all else.

I don't see any part at all updating live-in lists, and I can't find any place in particular that checks that all predecessors of an MBB has been visited. Given that the jumps are done at the end, it would have to be after that, but there is nothing there...

I am not quite sure how this all works, but it seems that EFLAGS is copied into a virtual register (or several - saved in CondRegs) which the users are supposed to rely on, which would then mean that EFLAGS is in fact never live into any MBB with a user found here anymore after this pass?

I made this patch based on this idea which "seems to work", but I am not at all sure this is correct, so please take a look...

The other idea I had was to simply recompute the live-in lists from scratch after everything else, possibly as a new method of LivePhysRegs.

Diff Detail

Event Timeline

jonpa created this revision.Dec 11 2019, 1:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 1:08 PM
craig.topper added inline comments.Jan 5 2020, 12:20 PM
llvm/lib/Target/X86/X86FlagsCopyLowering.cpp
549

I don't think we can do this here. Later we check for successor blocks with livein eflags and I believe we expect to be able to get back to the original MBB that started the worklist as second time if there is a loop. We don't put the initial block into the visited set until its queued as a successor. But we need the eflags livein in order to queue it.

554

Here the comments talk about visiting the block twice.

jonpa updated this revision to Diff 236709.Jan 7 2020, 3:51 PM
jonpa marked 3 inline comments as done.

Patch updated -- see inline comment for discussion.

llvm/lib/Target/X86/X86FlagsCopyLowering.cpp
549

Aha. My suggestion here did not update the live-in list for the original block, but it seems that this must then also be done at some point, which it is not...

It seems to work better then to do this when finding the successors, so that any successor with a live in EFLAGS gets it removed directly from the live-in list. This would then happen for MBB only inside a cycle when it is reached as a successor.

This seems good to me, while I still wonder if there would be any remaining cases of EFLAGS being live-in after this pass? If not, an alternative might be to just remove it from all live-in list at the end after all else.

This revision is now accepted and ready to land.Jan 8 2020, 12:12 PM
This revision was automatically updated to reflect the committed changes.