New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/compile: BLAS DdotSmall and Idamax regressions #14917
Comments
I'm pretty sure this has nothing to do with #14920. At least, the fix for that issue certainly doesn't apply here. before:
after:
The nil check in the "before" case can be removed because we have a nil check of &a[i] followed by a load from that pointer. The nil check is effectively merged into the load so you don't see it in the generated assembly. The nil check in the "after" case isn't being removed because it is checking the original slice ptr &a[0] each time. The load is still from &a[i] so the merging doesn't happen and the nil check must be explicit. In either case, it is kind of dumb to do nil checks on the pointer loaded from a slice. If the bounds check passes, the pointer is guaranteed to be non-nil. I'll see if I can whip up a fix. It might also be instructive to figure out what changed in the last 20 days about nil checks, I'm not sure what caused it. I don't see any difference in the inner loop between d37d3bd and tip, so I'm not sure why @btracey you saw performance improve at d37d3bd. |
Looks like the culprit is https://go-review.googlesource.com/20307 |
I'll send a CL to remove that rewrite and I'll figure it out how to remove the NilChecks completely. Now that we have use counts should be easier. |
CL https://golang.org/cl/21040 mentions this issue. |
That change may have fixed Ddot, but it does not fix the Idamax problem. Comparing go1.6 with
Should I open a different issue? |
Sorry about closing this prematurely. But yes, please open a different issue. There are two different underlying problems so it would be better to have them in different issues. |
No problem. Do you have a preferred title? |
cmd/compile: BLAS Idamax regression |
Idamax and Ddot small are showing regressions between 1.6 and gotip.
Reproducing:
While the Ddot regressions look small, there had been improvements as of the resolution of #14511 , so it's a significant regression since then. Ddot shows improvement between 1.6 and tip for the benchmarks with larger vector sizes (slices of size 100+ instead of 10).
/cc @randall77 @tzneal @dr2chase @josharian @brtzsnr
The text was updated successfully, but these errors were encountered: