Page MenuHomePhabricator

AMD Znver2 (Rome) Scheduler enablement
ClosedPublic

Authored by GGanesh on Aug 12 2019, 6:57 AM.

Details

Summary

The patch gives out the details of the znver2 scheduler model.
Rome is second generation zen family processor.
There are few improvements with respect to execution units, latencies and throughput when compared with znver1.
The tests that were present for znver1 for llvm-mca tool were replicated. The latencies, execution units, timeline and throughput information are updated for znver2.
I am working on another patch focusing on the information as mandated by exegesis.

Diff Detail

Event Timeline

GGanesh created this revision.Aug 12 2019, 6:57 AM
hfinkel added inline comments.
llvm/lib/Target/X86/X86ScheduleZnver2.td
108

Indent.

351

Commented out?

606

Can this "definitely wrong" thing be fixed?

629

Here, and below, some of these are commented out. Why? Can/should these just be removed?

1195

Commented out?

GGanesh updated this revision to Diff 225260.Oct 16 2019, 10:42 AM

The changes for review comments are incorporated.
The latency information in CLZERO, VZEROUPPER, MUL instructions are updated.

GGanesh updated this revision to Diff 225324.Oct 16 2019, 3:46 PM

Updated for review comments and latency modifications for MUL, vzeroupper, CLZERO instructions.

jdoerfert added inline comments.
llvm/lib/Target/X86/X86ScheduleZnver2.td
50

Nit: Three?

lebedev.ri added inline comments.Oct 19 2019, 12:45 PM
llvm/lib/Target/X86/X86ScheduleZnver2.td
91–92

But let LoadLatency = 4;.
Shouldn't this also be 4?

156

But ReadAdvance<ReadAfterVecLd is at 7?

GGanesh updated this revision to Diff 226091.Oct 22 2019, 3:11 PM

Updated the patch for review comments.

Not sure how this (any) sched model should be reviewed.
Perhaps it would help to post the exegesis reports.

FWIW i'd just merge this.

llvm/lib/Target/X86/X86ScheduleZnver2.td
180

This is correct?
Shouldn't it be 4+1 ?

Not sure how this (any) sched model should be reviewed.
Perhaps it would help to post the exegesis reports.

FWIW i'd just merge this.

I agree with Roman that there's not a good way to review this other than comparing with exegesis data. Sounds like from the initial description there is planned follow up with more data from exegesis. So I think this is fine to start with.

I agree on having a patch to enable exegesis. Will post that in couple of days. As mentioned, this is part of the initial plan as well.

hfinkel accepted this revision.Nov 19 2019, 6:50 AM

I agree on having a patch to enable exegesis. Will post that in couple of days. As mentioned, this is part of the initial plan as well.

It looks like everyone is in agreement: We should move forward with this, exegesis next as follow-up, and then updates from there as appropriate. Thus, LGTM.

This revision is now accepted and ready to land.Nov 19 2019, 6:50 AM

Do you have the commit rights?

This revision was automatically updated to reflect the committed changes.