Skip to content

Conversation

@pentp
Copy link
Contributor

@pentp pentp commented Apr 6, 2021

These were incorrectly grouped and thus sqrt PerfScores were way too low.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 6, 2021
result.insThroughput = PERFSCORE_THROUGHPUT_3C;
result.insLatency += PERFSCORE_LATENCY_12C;
result.insThroughput = PERFSCORE_THROUGHPUT_1C;
result.insLatency += PERFSCORE_LATENCY_4C;
Copy link
Member

@tannergooding tannergooding Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On https://uops.info/table.html, I see the following for Skylake:

sqrtsd   64, TP 1.00 / 4.50, LAT [≤13.0;≤19]
sqrtpd  128, TP 1.00 / 4.50, LAT [≤13.0;≤19]
sqrtpd  256, TP 1.00 / 9.00, LAT [≤13.0;≤20]


sqrtss   32, TP 1.00 / 3.00, LAT [≤12.0;≤13]
sqrtps  128, TP 1.00 / 3.00, LAT [≤12.0;≤13]
sqrtps  256, TP 1.00 / 6.00, LAT [≤12.0;≤13]

rsqrtss  32, TP 1.00 / 1.00, LAT [≤4.0;≤5]
rsqrtps 128, TP 1.00 / 1.00, LAT [≤4.0;≤5]
rsqrtps 256, TP 1.00 / 1.00, LAT [≤4.0;≤5]

rcpss    32, TP 1.00 / 1.00, LAT 4
rcpps   128, TP 1.00 / 1.00, LAT 4
rcpps   256, TP 1.00 / 1.00, LAT 4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular, it looks like the 256-bit variants can be twice as expensive on throughput (as least for non-reciprocal cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw that also when I checked the values, thought of even coding the argument size check, but then decided against it based on the fact that it wouldn't have any practical effect since the PerfScore is dominated by the larger latency value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, just thought I'd call it out 😄

It might be worth an up-for-grabs issue to get these correctly tracked. Given that uops.info has everything available in an xml file (https://uops.info/xml.html), it would likely be possible to have a small generator that fills in a metadata header file (much like is used for hwintrinsiclistxarch.h) so this can be automated and is trivial to update to newer or older micro-architectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with PerfScore code, but such an approach might be quite complicated - the uops.info measurements are made for each variant of encoding/reg-vs-mem/op-size and latencies are for each combination of input and output parameters. Currently PerfScore values are more of an art than science (e.g. latency for DIV is just set to the arithmetic mean of min/max, while in practice it might actually be on the lower end for 128:64 div as the upper 64bits are basically always zero) or the latency for MUL rdx:rax,reg is set to 4 (high half of output used) as it's the common use case, not 3 (only low half used) which is only rarely emitted.

@AndyAyersMS
Copy link
Member

@briansull PTAL

Copy link
Contributor

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good,
Thanks for your contribution

@briansull briansull merged commit 0cf4f15 into dotnet:main Apr 15, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 15, 2021
@pentp pentp deleted the sqrt-perfscore branch April 15, 2021 23:58
@ghost ghost locked as resolved and limited conversation to collaborators May 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants