Clean up pack reordering #5782
Clean up pack reordering #5782
Conversation
xu-shawn
force-pushed
the
cleanup_pack_ordering_2_pr
branch
4 times, most recently
from
75b6283 to
7e080de
Compare
xu-shawn
force-pushed
the
cleanup_pack_ordering_2_pr
branch
from
7e080de to
78852e1
Compare
There was a problem hiding this comment.
Overall it's a step in the right direction, but the crux is that the permutations are very coupled with the inference implementation, and this patch would make the connection very loose, by extracting the permutation definitions to a different class.
As a middle ground I'd propose simple permute/unpermute helper functions that take a raw memory block (128 bytes in this case), the size of elements being permuted (16 bytes in this case), and an array defining the permutation. The permutation should be specified in the FeatureTransformer class, preferably close to loading the weights.
src/nnue/nnue_feature_transformer.h
Outdated
| void permute_weights(Function order_fn) { | ||
| for (IndexType i = 0; i < HalfDimensions; i += Packing::process_chunk) | ||
| order_fn(&biases[i]); |
There was a problem hiding this comment.
I don't like how there's no indication that order_fn should be one a 2 specific member functions of Packing. Perhaps permute_for_packus_epi16 and unpermute_for_packus_epi16 should be modified to work on multiple permutation blocks and be called directly instead of this function.
Also fixes the strict aliasing violation in the current implementation
no functional change