Fix qb4 packw bias tail in neon#10787
Conversation
e8c02c7 to
476c60d
Compare
fbarchard
left a comment
fbarchard
left a comment
There was a problem hiding this comment.
wrong amount of bias values are copied.
the memcpy/memset version looks right.
| const int32x4_t b3 = vld1q_s32(&b[12]); | ||
| vst1q_s32((int32_t*)out + 12, b3); | ||
| b += 16; | ||
| if (n >= 16) { |
There was a problem hiding this comment.
this isnt right. you copy 16 but not the rest. it needs to be in a loop and copy n bias values.
ditto for scalar.
you can/should write the full NR (16 floats) but its important on the remainder to only read n to avoid out of bounds.
optional, but a good idea to clear the rest to zero.
There was a problem hiding this comment.
disreguard... you are doing the neon right. its a direct copy of 4 vectors when n >= 16
and memcpy/memset for remainder.
fbarchard
left a comment
fbarchard
left a comment
There was a problem hiding this comment.
looks good. I tried it on android and it runs smoothly
| const int32x4_t b3 = vld1q_s32(&b[12]); | ||
| vst1q_s32((int32_t*)out + 12, b3); | ||
| b += 16; | ||
| if (n >= 16) { |
There was a problem hiding this comment.
disreguard... you are doing the neon right. its a direct copy of 4 vectors when n >= 16
and memcpy/memset for remainder.
| ((uint32_t*) out)[12] = b[12]; | ||
| ((uint32_t*) out)[13] = b[13]; | ||
| ((uint32_t*) out)[14] = b[14]; | ||
| ((uint32_t*) out)[15] = b[15]; |
There was a problem hiding this comment.
this is fine and both this and memcpy(out, b, 16 * sizeof(uint32_t));
will vectorize to 4 vectors, but the unrolling exposes a pedantic compiler performance issue for very large NR ... e.g. NR=256 will be heavily unrolled and the compiler will spend a long time (more than 10 minutes) trying to compile this file. It eventually does the right thing, which is a few vectors.
3 participants
The kernel template allows for indexing past the end the bias, which can result in a SIGSEGV at packing.
Here we handle the tail with a a memcpy + memset with proper bounds.
Added a test for these cases as well.
Also confirmed with actual usecases on device it resolves a SIGSEGV.