SVE optimised float WSSJ kernel #2917
Conversation
rakshithgb-fujitsu
requested review from
Alexsandruss,
samir-nasibli and
Alexandr-Solovev
as code owners
|
@keeranroth have a look at this. |
There was a problem hiding this comment.
Can't comment too much on the algorithm, as I'm not familiar with SVM code. But it looks believable. There are just some style points I picked up on. Will let someone knowledgeable about the application area give some more guidance.
I can't help but feel that there is some instruction level parallelism being left on the table. A lot of the instructions are dependent. Having the masking, you might be able to duplicate the work, and simply select the result that you want at the end. I feel as though this implementation is going to be using only one pipeline at the moment. Would need profiling to confirm, though
| /* | ||
| * Contains optimizations for SVE. | ||
| */ |
There was a problem hiding this comment.
This comment isn't adding any information. If you want to add more information about what the algorithm is doing into the comment, that would be ideal. Otherwise remove it
| svint32_t Bj_vec = svdup_s32(-1); | ||
|
|
||
| // some constants used during optimization | ||
| // enum SVMVectorStatus low = 0x2 |
There was a problem hiding this comment.
Not sure where low would be defined before this, but maybe this isn't supposed to be a comment? The code below uses it, so I'm assuming this should be uncommented
There was a problem hiding this comment.
low is defined elsewhere, this comment basically reminds what the value of low is outside.
| } | ||
| else | ||
| { | ||
| DAAL_ASSERT((sign & (sign - 1)) == 0) // used to make sure sign is always having 1 bit set |
There was a problem hiding this comment.
Not sure what get sign returns, but this assert is also true when sign = 0, so the comment isn't correct. I suspect this might not be what you want to be checking on the result of getSign
There was a problem hiding this comment.
The idea was to keep this optimization done under check, since low = 0x2, if it were to ever change this debug assert would help. Where getSign is defined here -
|
|
||
| size_t j_cur = jStart; | ||
|
|
||
| for (j_cur; j_cur < jEnd; j_cur += w) |
There was a problem hiding this comment.
| for (j_cur; j_cur < jEnd; j_cur += w) | |
| for (; j_cur < jEnd; j_cur += w) |
We do think that there is more room for improvement, and we are exploring more ideas, we've finalized on this version for now and if we do optimize it further will raise another PR for it. If you do see any instruction level bottlenecks, please do point it out. |
|
Great to see specialization for algo |
|
/intelci: run |
This PR introduces performance optimizations for the WSSJ Float using SVE intrinsics, resulting in significant improvements for the SVM algorithms on ARM.
Key Improvements:
Boser Method: 22% performance gain, leading to faster computation and better resource utilization.
Thunder Method: 5% performance gain, enhancing efficiency in scenarios where this method is used.
Changes:
Code Updates: New SVE intrinsics based WSSJ Float kernel.
Impact:
Performance: Faster processing times and improved efficiency for SVM algorithms observed and documented on single core.
Performance on single core:
