This is an archive of the discontinued LLVM Phabricator instance.

[X86][SchedModels] Fix missing ReadAdvance for MULX and ADCX/ADOX (PR51494)
ClosedPublic

Authored by andreadb on Aug 19 2021, 6:25 AM.

Details

Summary

Before this patch, instructions MULX32rm and MULX64rm were missing a ReadAdvance for the implicit read of register EDX/RDX.
This patch fixes the issue, and it also adds a ReadAdvance for the implicit read of EFLAGS in ADCX/ADOX.

This patch also introduces a new SchedWrite for the MULX variants. By using those writes, we can get rid of most InstRW in the Zen models. The only exception is the Zen3 model, for which an InstRW is still required to override the RM variants of MULX.
Another minor advantage of using these new SchedWrites is that we can now correctly mark MULX as unsupported on models used by targets that don't feature BMI2.

While doing this minor refactoring, I noticed (and fixed) an inconsistency in the throughput numbers reported by zen1 models for MULX64. Specifically, the MULX64 variant didn't correctly declare resources, and it had an unrealistic throughput. That issue also affected the zen2 model (I suspect a copy-paste mistake; most definitions in zen2 seems copied straight from zen1 modulo a small rename of substring Zn to Zn2).
The new values now correctly match what is reported in the AMD SOG official document for family 17th.

Diff Detail

Event Timeline

andreadb created this revision.Aug 19 2021, 6:25 AM
andreadb requested review of this revision.Aug 19 2021, 6:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 6:25 AM
andreadb edited the summary of this revision. (Show Details)Aug 19 2021, 9:58 AM
andreadb edited the summary of this revision. (Show Details)Aug 19 2021, 10:06 AM

@lebedev.ri Are you OK with the znver changes?

llvm/lib/Target/X86/X86ScheduleZnver3.td
631

its weird that the rm variant uses the Zn3Multiplier pipe for an extra cycle......

Just checking: we don't have this problem with SBB?

@lebedev.ri Are you OK with the znver changes?

znver3 seems fine, thanks.

llvm/lib/Target/X86/X86ScheduleZnver3.td
631

Yes, but i'm pretty sure this is not a typo.
There might be modelling problems in mca/exegesis, so it might be spurious.

andreadb added inline comments.Aug 20 2021, 7:58 AM
llvm/lib/Target/X86/X86ScheduleZnver3.td
631

Yeah. That is basically the reason why I couldn't remove the InstRW from the znver3 model.

I also wonder why there is an extra Zn3Multiplier cycle only for the RM variants. Normally, you would expect the RM variants to semantically behave like a Load+RR sequence.

Maybe it was done intentionally, to artificially decrease the throughput of the MULX RM variants only (in order to better match the exegesis report).

lebedev.ri added inline comments.Aug 20 2021, 8:00 AM
llvm/lib/Target/X86/X86ScheduleZnver3.td
631

Maybe it was done intentionally, to artificially decrease the throughput of the MULX RM variants only (in order to better match the exegesis report).

Yep, that was my comment precisely.

Just checking: we don't have this problem with SBB?

Sorry. I missed that first question.

SBB was already fixed by this commit: https://reviews.llvm.org/rG7a1a35a1d1ae2e69769505c9f39910067c53d53b
That was the commit that fixed PR51318 and PR51322.

lebedev.ri accepted this revision.Aug 20 2021, 8:30 AM

Just checking: we don't have this problem with SBB?

Sorry. I missed that first question.

SBB was already fixed by this commit: https://reviews.llvm.org/rG7a1a35a1d1ae2e69769505c9f39910067c53d53b
That was the commit that fixed PR51318 and PR51322.

Aha, i thought it was.

LG to me, thank you.

@RKSimon ?

This revision is now accepted and ready to land.Aug 20 2021, 8:30 AM
RKSimon accepted this revision.Aug 20 2021, 8:33 AM

LGTM - cheers!