Page MenuHomePhabricator

X86InstrInfo: Do not stop on flag users between flag producers
AbandonedPublic

Authored by MatzeB on Thu, Sep 30, 11:02 AM.

Details

Summary

optimizeCompareInstr would stop searching if there are other EFLAG readers between the CmpInstr that we attempt to remove and the earlier instruction that we try to use instead. Example:

= SUB32rr %0, %1, implicit-def $eflags
...  we no longer stop when there are $eflag users here
CMP32rr %0, %1   ; will be removed
...

I don't see any reason why we cannot allow EFLAG readers between the two flag producers. Maybe it was an accident/artifact that wasn't obvious before the cleanups in D110857

Diff Detail

Event Timeline

MatzeB created this revision.Thu, Sep 30, 11:02 AM
MatzeB requested review of this revision.Thu, Sep 30, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 30, 11:02 AM
MatzeB retitled this revision from X86InstrInfo: Continue if there's other EFLAG readers between producers to X86InstrInfo: Do not stop on flag users between flag producers.Thu, Sep 30, 11:02 AM

I have concern about the live interval of eflags. If we killed the eflags after the first use, we are free to insert or schedule instructions that implicit-def eflags. Will these be affected if we keep eflags alive?

foad added a subscriber: foad.Fri, Oct 1, 6:21 AM
MatzeB added a comment.Fri, Oct 1, 9:51 AM

I have concern about the live interval of eflags. If we killed the eflags after the first use, we are free to insert or schedule instructions that implicit-def eflags. Will these be affected if we keep eflags alive?

When searching for candidates in the main search (the for loops in line 4298/4299) we never search further than the first instruction that modifies EFLAGS (see line 4351).

MatzeB added a comment.Fri, Oct 1, 9:52 AM

I have concern about the live interval of eflags. If we killed the eflags after the first use, we are free to insert or schedule instructions that implicit-def eflags. Will these be affected if we keep eflags alive?

If you are worried about pre-existing kill-flags? We remove them in line 4497 when we actually perform the optimization.

LGTM, but I'd like others to sign off the patch.

RKSimon accepted this revision.Thu, Oct 14, 2:16 AM

LGTM - if you can it might be better to merge this into D110857?

This revision is now accepted and ready to land.Thu, Oct 14, 2:16 AM

LGTM - if you can it might be better to merge this into D110857?

I kept them separate so D110857 would be an NFC change. Anyway I'll merge them now.