This is an archive of the discontinued LLVM Phabricator instance.

[X86] Canonicalize SGT/UGT compares with constants for JCC to use SGE/UGE to reduce the number of EFLAGs reads.
Changes PlannedPublic

Authored by RKSimon on Feb 20 2022, 1:22 PM.

Details

Summary

This extends D101074 so the SGT/UGT -> SGE/UGE canonicalization is used for conditional jumps as well as conditional moves and comparison results.

As discussed on PR48760, some EFLAG bits are treated independently which can require additional uops to merge together for certain Jcc cases.

I'm uncertain on how/whether to deal with the use-cr-result-of-dom-icmp-st.ll regressions, its not clear to me that this the "-cgp-icmp-eq2icmp-st flag" is actually enabled anywhere, and I get the impression that D60506 was principally a PPC combine that was made generic.

Diff Detail

Event Timeline

RKSimon created this revision.Feb 20 2022, 1:22 PM
RKSimon requested review of this revision.Feb 20 2022, 1:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2022, 1:22 PM

What CPUs require extra uops for Jcc? Last I knew Intel Core CPUs didn’t. Not sure about Atom or AMD.

AMD targets tend to split the flags register into subregisters, so reading fewer flags often means we avoid multiple reads.

RKSimon updated this revision to Diff 410277.Feb 21 2022, 4:27 AM

Remove some test code that I forgot about

pengfei added inline comments.Feb 21 2022, 4:32 AM
llvm/test/CodeGen/X86/use-cr-result-of-dom-icmp-st.ll
19

Regression?

RKSimon added inline comments.Feb 21 2022, 5:13 AM
llvm/test/CodeGen/X86/use-cr-result-of-dom-icmp-st.ll
19

kind of - its actually closer to the codegen we get without the -cgp-icmp-eq2icmp-st flag - as I mentioned in the summary, this flag isn't enabled anywhere afaict

I'll add an extra test run without the flag to show the diff

pengfei added inline comments.Feb 21 2022, 5:19 AM
llvm/test/CodeGen/X86/use-cr-result-of-dom-icmp-st.ll
19

Sorry, didn't notice it mentioned.

RKSimon updated this revision to Diff 410289.Feb 21 2022, 5:54 AM

rebase - with -cgp-icmp-eq2icmp-st disabled we still appear to have a regression

RKSimon planned changes to this revision.Feb 22 2022, 9:13 AM
lebedev.ri resigned from this revision.Jan 12 2023, 4:58 PM

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 4:58 PM
Herald added a subscriber: StephenFan. · View Herald Transcript