Take the hot loops off the shape that blocks vectorization - #510
Merged
Merged
Conversation
The gate passed at 188 of 188, and affinity propagation passed it at 1.003x on
one run and failed it at 0.947x on the next, on code that had not changed. Our
own time moved by two tenths of a percent across those runs and scikit-learn's
moved by fifteen, so the row was a coin toss rather than a win.
Looking for room in it found something that is not about that row at all.
Flow emits a range loop as
for (int32_t k = 0; (0 <= n) ? k < n : k > n; k += (0 <= n) ? 1 : -1)
so the stride is a runtime select and LLVM declines to vectorize the body. The
disassembly of affinity_propagation_fit held no vector instruction at all.
Writing the two inner loops as while loops with a constant step took the fit
from 5.23 ms to 2.97 ms and the function from zero vector instructions to 38.
That is Flow issue #1004, and until it is fixed the workaround is a while loop
in anything hot.
The same treatment on the kernel methods, plus two things worth doing anyway:
The kernel matrix is symmetric under every kernel the module has, so it is
computed over one triangle and mirrored, where it used to evaluate all n^2
entries and write each one through matrix_set. The kernel between two rows now
takes pointers and loops over the features with a constant step, which reaches
every caller including prediction. SVR's inner update, f = f + delta * row, is
a saxpy, so it calls one.
Local timings at -O3, against the last CI reading of the same rows:
svr 10.82 ms -> 1.22
nu_svr 6.61 -> 1.52
affinity_propagation 6.47 -> 2.97
svc 0.90 -> 0.15
linear_svr 0.44 -> 0.20
The gate passed again at 188 of 188, and the row at the bottom of it moved from affinity propagation to label spreading at 1.049x. The whole distribution moved down that run, on rows nobody had touched, which is the runner rather than the code. A row at 1.05x is the next coin toss, so these are the four narrowest. label_spreading propagated its distributions with a scalar triple loop that walked Y down a column of an array of row allocations, a cache miss per element, and allocated a fresh set of rows on every iteration. The propagation is a matrix product, so it is one sgemm against contiguous storage. 1.04 ms to 0.12. knn_regressor read every element of both designs through matrix_at, and ran over all k neighbours for every training point to find the slot to replace when it only has to look when a point actually displaces one. 1.24 ms to 0.53. isomap held its distances, geodesics and centering in arrays of row allocations. Floyd-Warshall now walks contiguous rows with the term that depends only on i and k lifted out, and the comparison written as a minimum so the inner loop holds no branch. The power iteration is one sgemm per pass over the whole component block. 6.22 ms to 4.04. local_outlier_factor and the label spreading affinity get their distance loops as while loops, for the reason the last commit gives: Flow emits a range loop with a stride chosen at runtime, and LLVM will not vectorize that. Flow issue #1004.
gamma_regressor was among the narrowest rows left at 1.43x. Its gradient loop and its weight update are while loops with a constant step now, for the reason the two commits before this one give. 0.68 ms to 0.49 on this machine, and poisson and tweedie share the loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The wide matrix gate passed at 188 of 188, and
affinity_propagationpassed itat 1.003x on one run and failed it at 0.947x on the next, on code that had not
changed. Our own time moved by two tenths of a percent across those runs and
scikit-learn's moved by fifteen. That row needed real headroom rather than luck.
What the search for it found
Flow emits a range loop as
The stride is a runtime select, so LLVM will not vectorize the body.
affinity_propagation_fitdisassembled to zero vector instructions. Writing itstwo inner loops as while loops with a constant step took the fit from 5.23 ms to
2.97 ms and the function to 38 vector instructions.
Filed upstream as flooooooooooow/flow#1004. Until it is fixed, anything hot in
this library wants a while loop.
What is in here
The same treatment on the kernel methods, plus two things worth doing anyway:
computed over one triangle and mirrored, where it used to evaluate all n^2
entries and write each one through
matrix_set.constant step, which reaches every caller including prediction.
f = f + delta * row, is a saxpy, so it calls one.Measurements
Local,
-O3, against the last CI reading of the same rows:The runner is the authority, and the gate on this PR is the measurement.
Verification
test_opt_kernelsvc_fit,test_opt_svc_predict,test_expanded_estimators,test_advanced_estimators,test_sparse_estimators,test_estimators,examples/svm_demoandexamples/full_demoall exit 0.