-
Notifications
You must be signed in to change notification settings - Fork 187
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
Increasing #elements/#pointers in matrix arg struct to 6 #929
Conversation
break CI heavily, but some initial tests (vanilla GEMM, simple eltwise) are expected to go through
…ge, op args are still 32byte
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The only general comment is that in this PR the "magic constants" are "just" adjusted properly (vs adding some named constants throughout). For now we should be good as discussed earlier (as most likely we will not touch this again) but maybe as an enhancement we can add an issue to keep track of this (and link to this PR such that the touch points are obvious)
Yes, I'm right now working on a change set to make them all sizeof(libxsmm_matrix_arg) and some calculations, will hopefully land tonight PT in the PR and then I will merge. The only thing it won't fix is the equation scratch stuff as this still works with magic pointers. |
Yeah, not super happy about the constants either, but I guess that's more than just a little refactoring that will need its own PR later. |
My understanding is application code change is required only if GEMM PREFETCH flag is used. Since I am not using any prefetch flags, I don't need any change in extension code to use new APIs. @alheinecke is my understanding correct? |
Yepp, that's correct. And of course since the recommendation is to memset to zero all the parameter structs before handing them to the JIT'ed kernels the zero-ing is automatically adjusted as one uses However, since this PR is touching the guts of LIBXSMM, I recommend updating to latest main after merge ASAP and testing if everything is still working as expected. @egeor , @rengolin : I have just pushed more changes which eliminate ~90% of the magic number usages and makes future changes to the function parameter structs very easy. @egeor : feel free to adopt the define scheme to remove the other magic numbers as well. As we all agree this is not urgent and after merge I will create an issue with a link to the one define I have already created within this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just reviewed again in the GUI, looks good
@alheinecke Just tested llm example from tpp pytorch extension with latest libxsmm main, everything looks good! |
In order to support dynamic LDs we need to get more arguments into the kernel. As we are at the limit of elements this PR increases the number arguments from four to six, but no support for dynamic LDs is added. The goal of the PR is to enable apps using libxsmm to adopt to the new API (which should be zero code change) and to test with the new version of libxsmm