[compiler][arm64] Instruction latencies. [v8/v8 : main]
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
Done ;)
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15b9b8c9c90000
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream-main.crossbench complete.
- ml: base median = 260.0517451286581 -> patched median = 236.52338799036923
- stanford-crypto-aes: base median = 844.1923446837709 -> patched median = 794.9815443925895
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1757142ec90000
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Darius Mercadier@dmerc...@chromium.org Would you mind kicking off one/some pinpoints..?
Done ;)
Can we try just --turbo-instruction-scheduling? This patch would change the default behaviour.
Related details
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
Darius Mercadier@dmerc...@chromium.org Would you mind kicking off one/some pinpoints..?
Sam Parker-HaynesDone ;)
Can we try just --turbo-instruction-scheduling? This patch would change the default behaviour.
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14255d9ec90000
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12d3e6e1c90000
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Is this okay, performance wise? I would prefer to ignore the experimental results for now, as the 'backward scheduling' patch should revert the troublesome code.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
Is this okay, performance wise? I would prefer to ignore the experimental results for now, as the 'backward scheduling' patch should revert the troublesome code.
Mmmh not ideal, but if the CL still makes sense, then I think it's ok. Can you convince me in a few sentences that the CL makes sense (I haven't really looked at it)? :D
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Darius MercadierIs this okay, performance wise? I would prefer to ignore the experimental results for now, as the 'backward scheduling' patch should revert the troublesome code.
Mmmh not ideal, but if the CL still makes sense, then I think it's ok. Can you convince me in a few sentences that the CL makes sense (I haven't really looked at it)? :D
Maybe a few points:
- Lots of long latency operations can cause greater register pressure.
- Shorter latencies actually should mean the scheduler spends less time trying to schedule.
- We currently do not have any Neon instructions and in my downstream testing, on an Arm Linux machine, this patch gave double digit improvements on many simd kernels.
- Pinpoint shows no difference in the default mode, which I'm really surprised about, but it doesn't appear to be doing any harm.
Related details
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 3 comments![]( "Open in Gerrit")
I'm a bit surprised by some of the new latencies. And a bit surprised that have less precise latencies (only 3 different ones for scalar vs 8 different ones before). Can you explain a bit (maybe in the commit message actually) why the new latencies make more sense? Were the old ones outdated? Or were they never actually correct? Or... ?
The commit message says "Probably the most important part is adding costs for Neon
instructions." ==> maybe it would make sense to split this change in 2 part then: one adding the Neon latencies and one updating the scalar ones?
case kArm64Idiv:
case kArm64Idiv32:
case kArm64Udiv:
case kArm64Udiv32:
case kArm64Imod:
case kArm64Imod32:
case kArm64Umod:
case kArm64Umod32:
return kVeryLongScalarLatency;That's much lower than before; are modern Arm CPUs really that fast to do division?
case kArm64Float64Mod:That's a surprising one as well!
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 2 comments![]( "Open in Gerrit")
I'm a bit surprised by some of the new latencies. And a bit surprised that have less precise latencies (only 3 different ones for scalar vs 8 different ones before). Can you explain a bit (maybe in the commit message actually) why the new latencies make more sense? Were the old ones outdated? Or were they never actually correct? Or... ?
The commit message says "Probably the most important part is adding costs for Neon
instructions." ==> maybe it would make sense to split this change in 2 part then: one adding the Neon latencies and one updating the scalar ones?
Yes, sorry the message was a little terse, I've basically had the same discussion downstream and should have dumped the reasoning somewhere 😊 The commit message has been updated.
Basically the latencies do not reflect reality but make it harder for scheduler to make decisions that increase register pressure. But I'm also working on a patch so the scheduler becomes aware of that too.
I can split the patch if that's preferred.
case kArm64Idiv:
case kArm64Idiv32:
case kArm64Udiv:
case kArm64Udiv32:
case kArm64Imod:
case kArm64Imod32:
case kArm64Umod:
case kArm64Umod32:
return kVeryLongScalarLatency;That's much lower than before; are modern Arm CPUs really that fast to do division?
No, I think most cores are actually 12 cycles or so, but I've found that in general having higher latencies just means higher register pressure and spills/refills end up being more expensive than a possible a few stall cycles.
Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 4 comments![]( "Open in Gerrit")
Sam Parker-HaynesI'm a bit surprised by some of the new latencies. And a bit surprised that have less precise latencies (only 3 different ones for scalar vs 8 different ones before). Can you explain a bit (maybe in the commit message actually) why the new latencies make more sense? Were the old ones outdated? Or were they never actually correct? Or... ?
The commit message says "Probably the most important part is adding costs for Neon
instructions." ==> maybe it would make sense to split this change in 2 part then: one adding the Neon latencies and one updating the scalar ones?
Yes, sorry the message was a little terse, I've basically had the same discussion downstream and should have dumped the reasoning somewhere 😊 The commit message has been updated.
Basically the latencies do not reflect reality but make it harder for scheduler to make decisions that increase register pressure. But I'm also working on a patch so the scheduler becomes aware of that too.
I can split the patch if that's preferred.
Ok thanks for the additional context.. Yea, I've also noticed that the scheduler has a tendency to increase register pressure and my very naive [attempt](https://crrev.com/c/7612927) to improve this didn't work, so thanks for spending more time to improve things here! :)
I can't say that I'm a huge fan of this CL though, because it feels like you don't fully know where you're going, and you're just hoping that more coarse latencies would be better. I'd prefer to keep precise(ish) latencies, and make the scheduler approximate them afterwards with something like
```
int ExactLatencyToApproxForScheduling(int latency) {
if (latency <= 1) return 1;
if (latency <= 3) return 2;
return 5;
}
```
So that we still have an opportunity to make use of the exact latency (to break ties for instance or something like that).
Additionally, this would be a less intrusive change, which is better given that as far as I understand you're unsure yet if your work will really pay off.
Another point: you're only updating arm64 with this new model, which means that the scheduler will do different things on arm64 and other architectures (until this CL is ported to other architectures), which isn't great.
case kArm64Idiv:
case kArm64Idiv32:
case kArm64Udiv:
case kArm64Udiv32:
case kArm64Imod:
case kArm64Imod32:
case kArm64Umod:
case kArm64Umod32:
return kVeryLongScalarLatency;Sam Parker-HaynesThat's much lower than before; are modern Arm CPUs really that fast to do division?
No, I think most cores are actually 12 cycles or so, but I've found that in general having higher latencies just means higher register pressure and spills/refills end up being more expensive than a possible a few stall cycles.
Acknowledged
That's a surprising one as well!
Acknowledged
default:It could make sense to remove the `default` so that new opcodes aren't assumed to have DefautScalarLatency.
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Sam Parker-HaynesI'm a bit surprised by some of the new latencies. And a bit surprised that have less precise latencies (only 3 different ones for scalar vs 8 different ones before). Can you explain a bit (maybe in the commit message actually) why the new latencies make more sense? Were the old ones outdated? Or were they never actually correct? Or... ?
The commit message says "Probably the most important part is adding costs for Neon
instructions." ==> maybe it would make sense to split this change in 2 part then: one adding the Neon latencies and one updating the scalar ones?
Darius MercadierYes, sorry the message was a little terse, I've basically had the same discussion downstream and should have dumped the reasoning somewhere 😊 The commit message has been updated.
Basically the latencies do not reflect reality but make it harder for scheduler to make decisions that increase register pressure. But I'm also working on a patch so the scheduler becomes aware of that too.
I can split the patch if that's preferred.
Ok thanks for the additional context.. Yea, I've also noticed that the scheduler has a tendency to increase register pressure and my very naive [attempt](https://crrev.com/c/7612927) to improve this didn't work, so thanks for spending more time to improve things here! :)
I can't say that I'm a huge fan of this CL though, because it feels like you don't fully know where you're going, and you're just hoping that more coarse latencies would be better. I'd prefer to keep precise(ish) latencies, and make the scheduler approximate them afterwards with something like
```
int ExactLatencyToApproxForScheduling(int latency) {
if (latency <= 1) return 1;
if (latency <= 3) return 2;
return 5;
}
```
So that we still have an opportunity to make use of the exact latency (to break ties for instance or something like that).Additionally, this would be a less intrusive change, which is better given that as far as I understand you're unsure yet if your work will really pay off.
Another point: you're only updating arm64 with this new model, which means that the scheduler will do different things on arm64 and other architectures (until this CL is ported to other architectures), which isn't great.
I'd prefer to keep precise(ish) latencies, and make the scheduler approximate them afterwards.
Okay.
until this CL is ported to other architectures
Sorry, I don't follow! Why would arm64 latencies be ported to other architectures?
Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
Sorry, I don't follow! Why would arm64 latencies be ported to other architectures?
Presumably you're going to work on the scheduler to make it work better and you'll make the assumption that you're dealing with the "new" arm64 latencies, so whatever you do might not be good for other architectures that didn't get the "latency update".
Related details
- Sam Parker-Haynes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
I think they should be orthagonal. With the exception of backward scheduling, all of the other changes will be controlled by 'models' supplied by the backends and those values can be set so that the status quo is maintained, as done in https://chromium-review.googlesource.com/c/v8/v8/+/7994015. That patch removes the one thing that could cause issues for arm64 and other backends so I need to think further about how that would be an opt-in.
Maybe it would just be best to land this patch after everything else? I don't want anyone thinking I've rewritten the scheduler to just benefit arm64!
Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/105cee8c290000
Related details
- Darius Mercadier
- Sam Parker-Haynes
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12491dca290000
Sam Parker-Haynes (Gerrit)
Sam Parker-Haynes added 1 comment![]( "Open in Gerrit")
Hi Darius, I've rewritten this so we're using accurate(ish) latencies.
Related details
- Darius Mercadier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |