Use 'const' pointers in kernel APIs. by fgvanzee · Pull Request #722 · flame/blis

fgvanzee · 2023-02-10T00:02:32Z

Details:

Qualified all input-only data pointers in the various kernel APIs with the const keyword while also removing restrict from those kernel APIs. (Use of restrict was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures.
Templatized the definitions of copys_mxn and xpbys_mxn static inline functions.
Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement).
Removed some unused kernel code left in old directories.
In common.mk, grep for "#include" instead of "\#include". Not sure why the backslash is suddenly giving a "grep: warning: stray \ before #" message, but removing the backslash seems to fix the issue.

@devinamatthews Note that this does not yet contain the switchover to void*. I wanted to do it in two phases in case anything went wrong.

Details: - Qualified all input-only data pointers in the various kernel APIs with the 'const' keyword while also removing 'restrict' from those kernel APIs. (Use of 'restrict' was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures. - Templatized the definitions of copys_mxn and xpbys_mxn static inline functions. - Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement). - Removed some unused kernel code left in 'old' directories. - In common.mk, grep for "#include" instead of "\#include". Not sure why the backslash is suddenly giving a "grep: warning: stray \ before #" message, but removing the backslash seems to fix the issue.

leekillough · 2023-02-10T04:06:58Z

If I may respectfully ask, what is the reason for removing restrict, and why wasn't const already used for pointers to read-only data?

devinamatthews · 2023-02-10T05:04:52Z

Re restrict, it's not really needed in the function prototypes or typedefs, since it only has an effect on the generated code when used in the definition. True, one could argue it serves a purpose as defining a user contract, but it's not quite nuanced enough to do that precisely for the kernels anyways. It is still used when writing kernel definitions usually.

Re const, the codebase didn't use const at all until I started putting it in a year ago or so. This is just an evolution of those efforts.

leekillough · 2023-02-10T05:25:33Z

A caller is not affected by knowing whether two declared pointer parameters are aliased (or declared restrict), unless one of them is read-only (pointer to const) and the other is read-write, and the caller reads the read-only one before and after the call, and because it knows of the restrict in the parameter declaration, it can assume that the read-only one was not affected during the call. But that only occurs in a very small number of real cases.

I am glad you're updating the appropriate use of const where it wasn't used before.

[edit: emphasis in italics]

devinamatthews · 2023-02-10T05:54:57Z

A caller is not affected by knowing whether two declared pointer parameters are aliased (or declared restrict), unless one of them is read-only (pointer to const) and the other is read-write, and the caller reads the read-only one before and after the call, and because it knows of the restrict in the parameter declaration, it can assume that the read-only one was not affected during the call. But that only occurs in a very small number of real cases.

Well, restrict on an interface is only a pinky-swear to the compiler. The implementation can ignore that restriction (same with const).

leekillough · 2023-02-10T06:52:37Z

Also, the caller can only assume that the const * restrict addressed parameter was not modified if its address was not "exposed" globally or through some other less restrictive call elsewhere (so-called "address exposed"). So the benefit of restrict is highly restricted.

Except in the innermost kernels, I agree that the utility of restrict is limited.

fgvanzee · 2023-02-10T17:27:19Z

For anyone not following the Discord discussion, these CI tests are failing because I removed the \ from:

REF_KER_HEADERS := $(shell $(GREP) "\#include" $(REF_KER_SRC) | ... )

in common.mk because as of grep 3.8, that backslash is causing the following warning:

grep: warning: stray \ before #" message

The warning is obviously undesired, but even more undesired is the build system breaking altogether. So I'm trying to figure out a way to satisfy both pre- and post-3.8 greps.

I tried switching from double to single quotes around #include, but that didn't work.

leekillough · 2023-02-10T17:42:58Z

I believe that # must be escaped because otherwise gmake assumes it's the start of the comment and ignores the rest of the line, obviously leading to a syntax error.

fgvanzee · 2023-02-10T17:59:03Z

I believe that # must be escaped because otherwise gmake assumes it's the start of the comment and ignores the rest of the line, obviously leading to a syntax error.

That kind of makes sense, but GNU make works as expected with:

REF_KER_HEADERS := $(shell $(GREP) "#include" $(REF_KER_SRC) | ... )

when using grep 3.8. (Double vs single quotes doesn't seem to matter here.) So... I dunno.

fgvanzee · 2023-02-10T18:47:50Z

While I don't like it, the only band-aid I could think of is to redirect stderr to /dev/null for that particular command sequence. (See #723.)

fgvanzee · 2023-02-11T00:01:44Z

@nisanthmp Please try compiling the power kernels in this ~~commit~~ PR to make sure I didn't break anything. If you get any errors (or warnings), please report them here so I can fix them.

nisanthmp · 2023-02-13T09:37:41Z

@fgvanzee
I'm getting the following errors:

In file included from kernels/power10/3/vector_int_macros.h:37,
from kernels/power10/3/bli_sgemm_power10_mma.c:35:
kernels/power10/3/bli_sgemm_power10_mma.c: In function ‘bli_sgemm_power10_mma_8x16’:
./frame/include//bli_edge_case_macro_defs.h:50:54: error: ‘cs_c’ undeclared (first use in this function); did you mean ‘_cs_c’?
50 | PASTEMAC(ch,ctype) _ct[ BLIS_STACK_BUF_MAX_SIZE
| ^~~~
./frame/include//bli_edge_case_macro_defs.h:78:9: note: in expansion of macro ‘GEMM_UKR_SETUP_CT_PRE’
78 | const bool _use_ct = ( row_major ? cs_c != 1 : rs_c != 1 ) ||
| ^~~~~~~~~~~~~~~~~~~~~
kernels/power10/3/bli_sgemm_power10_mma.c:77:5: note: in expansion of macro ‘GEMM_UKR_SETUP_CT’
77 | GEMM_UKR_SETUP_CT( s, 8, 16, true );
| ^~~~~~~~~~~~~~~~~
compilation terminated due to -Wfatal-errors.
make: *** [Makefile:695: obj/power/kernels/power10/3/bli_sgemm_power10_mma.o] Error 1
make: *** Waiting for unfinished jobs....
In file included from kernels/power10/3/vector_int_macros.h:37,
from kernels/power10/3/bli_dgemm_power10_mma.c:36:
kernels/power10/3/bli_dgemm_power10_mma.c: In function ‘bli_dgemm_power10_mma_8x8’:
./frame/include//bli_edge_case_macro_defs.h:50:54: error: ‘cs_c’ undeclared (first use in this function); did you mean ‘_cs_c’?
50 | PASTEMAC(ch,ctype) _ct[ BLIS_STACK_BUF_MAX_SIZE
| ^~~~
./frame/include//bli_edge_case_macro_defs.h:78:9: note: in expansion of macro ‘GEMM_UKR_SETUP_CT_PRE’
78 | const bool _use_ct = ( row_major ? cs_c != 1 : rs_c != 1 ) ||
| ^~~~~~~~~~~~~~~~~~~~~
kernels/power10/3/bli_dgemm_power10_mma.c:84:5: note: in expansion of macro ‘GEMM_UKR_SETUP_CT’
84 | GEMM_UKR_SETUP_CT( d, 8, 8, true );
| ^~~~~~~~~~~~~~~~~
compilation terminated due to -Wfatal-errors.

Details: - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels.

fgvanzee · 2023-02-13T16:09:48Z

Thank you, @nisanthmp. I think I've fixed the errors that you reported. Could you please confirm there are no others?

nisanthmp · 2023-02-13T20:14:11Z

@fgvanzee There are no other compilation errors when configured for power family, ie power9 and power10 configurations

fgvanzee · 2023-02-13T21:01:17Z

@fgvanzee There are no other compilation errors when configured for power family, ie power9 and power10 configurations

Wonderful. Thanks for your help!

angsch · 2023-02-21T08:17:49Z

Cosmetics: Some of the non-reference kernels received bonus \. For example:

blis/kernels/armsve/3/bli_gemm_armsve_asm_d2vx10_unindexed.c

Lines 46 to 57 in 93c63d1

    
           void bli_dgemm_armsve_asm_2vx10_unindexed 
        
                ( 
        
                        dim_t      m, \ 
        
                        dim_t      n, \ 
        
                        dim_t      k, \ 
        
                  const double*    alpha, \ 
        
                  const double*    a, \ 
        
                  const double*    b, \ 
        
                  const double*    beta, \ 
        
                        double*    c, inc_t rs_c0, inc_t cs_c0, \ 
        
                        auxinfo_t* data, \ 
        
                  const cntx_t*    cntx  \

Apply flame#722 to RISC-V

Details: - Qualified all input-only data pointers in the various kernel APIs with the 'const' keyword while also removing 'restrict' from those kernel APIs. (Use of 'restrict' was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures. - Templatized the definitions of copys_mxn and xpbys_mxn static inline functions. - Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement). - Removed some unused kernel code left in 'old' directories. - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels.

Details: - Updated common.mk so that when --disable-shared option is given to configure: 1. The -fPIC compiler flag is omitted from the individual configuration family members' CPICFLAGS variables (which are initialized in each subconfig's make_defs.mk file); and 2. The BUILD_SYMFLAGS variable, which contains compiler flags needed to control the symbol export behavior, is left blank. - The net result of these changes is that flags specific to shared library builds are only used when a shared library is actually scheduled to be built. Thanks to Nick Knight for reporting this issue. - CREDITS file update. - (cherry picked from commit 5f84130) Updated configure to pass all shellcheck checks. (#729) Details: - Modified configure so that it passes all 'shellcheck' checks, disabling ones which we violate but which are just stylistic, or are special cases in our code. - Miscellaneous other minor changes, such as rearranged redirections in long sed/perl pipes to look more natural. - Whitespace tweaks. - (cherry picked from 72c37eb) Fixed bugs in scal2v ref kernel when alpha == 1. (#728) Details: - Fixed a typo bug in ref_kernels/1/bli_scal2v_ref.c where the conditional that was supposed to be checking for cases when alpha is equal to 1.0 (so that copyv could be used instead of scal2v) was instead erroneously comparing alpha against 0.0. - Fixed another bug in the same function whereby BLIS_NO_CONJUGATE was erroneously being passed into copyv instead of the kernel's conjx parameter. This second bug was inert, however, due to the first bug since the "alpha == 0.0" case was already being handled, resulting in the code block never executing. - (cherry picked from 60f3634) Use 'void*' datatypes in kernel APIs. (#727) Details: - Migrated all kernel APIs to use void* pointers instead of float*, double*, scomplex*, and dcomplex* pointers. This allows us to define many fewer kernel function pointer types, which also makes it much easier to know which function pointer type to use at any given time. (For example, whereas before there was ?axpyv_ker_ft, ?axpyv_ker_vft, and axpyv_ker_vft, now there is just axpyv_ker_ft, which is equivalent so what axpyv_ker_vft used to be.) - Refactored how kernel function prototypes and kernel function types are defined so as to reduce redundant code. Specifically, the function signatures (excluding cntx_t* and, in the case of level-3 microkernels, auxinfo_t*) are defined in new headers named, for example, bli_l1v_ker_params.h. Those signatures are reused via macro instantiation when defining both kernel prototypes and kernel function types. This will hopefully make it a little easier to update, add, and manage kernel APIs going forward. - Updated all reference kernels according to the aforementioned switch to void* pointers. - Updated all optimzied kernels according to the aforementioned switch to void* pointers. This sometimes required renaming variables, inserting typecasting so that pointer arithmetic could continue to function as intended, and related tweaks. - Updated sandbox/gemmlike according to the aforementioned switch to void* pointers. - Renamed: - frame/1/bli_l1v_ft_ker.h -> frame/1/bli_l1v_ker_ft.h - frame/1f/bli_l1f_ft_ker.h -> frame/1f/bli_l1f_ker_ft.h - frame/1m/bli_l1m_ft_ker.h -> frame/1m/bli_l1m_ker_ft.h - frame/3/bli_l1m_ft_ukr.h -> frame/3/bli_l1m_ukr_ft.h - frame/3/bli_l3_sup_ft_ker.h -> frame/3/bli_l3_sup_ker_ft.h to better align with naming of neighboring files. - Added the missing "void* params" argument to bli_?packm_struc_cxk() in frame/1m/packm/bli_packm_struc_cxk.c. This argument is being passed into the function from bli_packm_blk_var1(), but wasn't being "caught" by the function definition itself. The function prototype for bli_?packm_struc_cxk() also needed updating. - Reordered the last two parameters in bli_?packm_struc_cxk(). (Previously, the "void* params" was passed in after the "const cntx_t* cntx", although because of the above bug the params argument wasn't actually present in the function definition.) - (cherry picked from fab18dc) Use 'const' pointers in kernel APIs. (#722) Details: - Qualified all input-only data pointers in the various kernel APIs with the 'const' keyword while also removing 'restrict' from those kernel APIs. (Use of 'restrict' was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures. - Templatized the definitions of copys_mxn and xpbys_mxn static inline functions. - Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement). - Removed some unused kernel code left in 'old' directories. - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels. - (cherry picked from 93c63d1)

Details: - Updated common.mk so that when --disable-shared option is given to configure: 1. The -fPIC compiler flag is omitted from the individual configuration family members' CPICFLAGS variables (which are initialized in each subconfig's make_defs.mk file); and 2. The BUILD_SYMFLAGS variable, which contains compiler flags needed to control the symbol export behavior, is left blank. - The net result of these changes is that flags specific to shared library builds are only used when a shared library is actually scheduled to be built. Thanks to Nick Knight for reporting this issue. - CREDITS file update. - (cherry picked from 5f84130) Updated configure to pass all shellcheck checks. (#729) Details: - Modified configure so that it passes all 'shellcheck' checks, disabling ones which we violate but which are just stylistic, or are special cases in our code. - Miscellaneous other minor changes, such as rearranged redirections in long sed/perl pipes to look more natural. - Whitespace tweaks. - (cherry picked from 72c37eb) Fixed bugs in scal2v ref kernel when alpha == 1. (#728) Details: - Fixed a typo bug in ref_kernels/1/bli_scal2v_ref.c where the conditional that was supposed to be checking for cases when alpha is equal to 1.0 (so that copyv could be used instead of scal2v) was instead erroneously comparing alpha against 0.0. - Fixed another bug in the same function whereby BLIS_NO_CONJUGATE was erroneously being passed into copyv instead of the kernel's conjx parameter. This second bug was inert, however, due to the first bug since the "alpha == 0.0" case was already being handled, resulting in the code block never executing. - (cherry picked from 60f3634) Use 'void*' datatypes in kernel APIs. (#727) Details: - Migrated all kernel APIs to use void* pointers instead of float*, double*, scomplex*, and dcomplex* pointers. This allows us to define many fewer kernel function pointer types, which also makes it much easier to know which function pointer type to use at any given time. (For example, whereas before there was ?axpyv_ker_ft, ?axpyv_ker_vft, and axpyv_ker_vft, now there is just axpyv_ker_ft, which is equivalent so what axpyv_ker_vft used to be.) - Refactored how kernel function prototypes and kernel function types are defined so as to reduce redundant code. Specifically, the function signatures (excluding cntx_t* and, in the case of level-3 microkernels, auxinfo_t*) are defined in new headers named, for example, bli_l1v_ker_params.h. Those signatures are reused via macro instantiation when defining both kernel prototypes and kernel function types. This will hopefully make it a little easier to update, add, and manage kernel APIs going forward. - Updated all reference kernels according to the aforementioned switch to void* pointers. - Updated all optimzied kernels according to the aforementioned switch to void* pointers. This sometimes required renaming variables, inserting typecasting so that pointer arithmetic could continue to function as intended, and related tweaks. - Updated sandbox/gemmlike according to the aforementioned switch to void* pointers. - Renamed: - frame/1/bli_l1v_ft_ker.h -> frame/1/bli_l1v_ker_ft.h - frame/1f/bli_l1f_ft_ker.h -> frame/1f/bli_l1f_ker_ft.h - frame/1m/bli_l1m_ft_ker.h -> frame/1m/bli_l1m_ker_ft.h - frame/3/bli_l1m_ft_ukr.h -> frame/3/bli_l1m_ukr_ft.h - frame/3/bli_l3_sup_ft_ker.h -> frame/3/bli_l3_sup_ker_ft.h to better align with naming of neighboring files. - Added the missing "void* params" argument to bli_?packm_struc_cxk() in frame/1m/packm/bli_packm_struc_cxk.c. This argument is being passed into the function from bli_packm_blk_var1(), but wasn't being "caught" by the function definition itself. The function prototype for bli_?packm_struc_cxk() also needed updating. - Reordered the last two parameters in bli_?packm_struc_cxk(). (Previously, the "void* params" was passed in after the "const cntx_t* cntx", although because of the above bug the params argument wasn't actually present in the function definition.) - (cherry picked from fab18dc) Use 'const' pointers in kernel APIs. (#722) Details: - Qualified all input-only data pointers in the various kernel APIs with the 'const' keyword while also removing 'restrict' from those kernel APIs. (Use of 'restrict' was maintained in kernel implementations, where appropriate.) This affected the function pointer types defined for all of the kernels, their prototypes, and the reference and optimized kernel definitions' signatures. - Templatized the definitions of copys_mxn and xpbys_mxn static inline functions. - Minor whitespace and style changes (e.g. combining local variable declaration and initialization into a single statement). - Removed some unused kernel code left in 'old' directories. - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels. - (cherry picked from 93c63d1)

fgvanzee self-assigned this Feb 10, 2023

grep warning: single -> double quotes.

9f4b121

fgvanzee mentioned this pull request Feb 10, 2023

Redirect grep stderr to /dev/null in common.mk. #723

Merged

Revert to double quotes for grep.

a33a95a

fgvanzee added 11 commits February 10, 2023 15:35

Merge branch 'master' into const_kernel_apis

a82ea10

Fixed type signatures for knl ukernels.

e259241

More type and syntax fixes.

dd07b0e

Fix type error in penryn axpyv kernel.

b9a8078

Fixes to level-1f penryn kernels.

e429e75

Yet more warnings fixed.

eae1bde

Fixes to armv8a kernels.

ac02809

More warnings/errors fixed for armv8a kernels.

1aee102

More syntax error fixes for armv8a kernels.

dfa3a38

Ugh.

4ad1ddc

Fixed probable warnings in power10, bgq kernels.

54e6b1e

Fixed missing cs_c in power10 s/dgemm ukernels.

cd46414

Details: - Thanks to Nisanth M P for helping to validate changes to the power10 microkernels.

fgvanzee merged commit 93c63d1 into master Feb 20, 2023

fgvanzee deleted the const_kernel_apis branch February 20, 2023 17:14

fgvanzee mentioned this pull request Feb 20, 2023

Use void* datatypes in kernel APIs. #727

Merged

angsch added a commit to angsch/blis that referenced this pull request Feb 21, 2023

Make pointers 'const'

a9c9cd4

Apply flame#722 to RISC-V

angsch added a commit to angsch/blis that referenced this pull request Feb 24, 2023

Make pointers 'const'

550d51c

Apply flame#722 to RISC-V

angsch added a commit to angsch/blis that referenced this pull request Mar 25, 2023

Make pointers 'const'

fa9ff55

Apply flame#722 to RISC-V

Conversation

fgvanzee commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leekillough commented Feb 10, 2023

Uh oh!

devinamatthews commented Feb 10, 2023

Uh oh!

leekillough commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devinamatthews commented Feb 10, 2023

Uh oh!

leekillough commented Feb 10, 2023

Uh oh!

fgvanzee commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leekillough commented Feb 10, 2023

Uh oh!

fgvanzee commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgvanzee commented Feb 10, 2023

Uh oh!

fgvanzee commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nisanthmp commented Feb 13, 2023

Uh oh!

fgvanzee commented Feb 13, 2023

Uh oh!

nisanthmp commented Feb 13, 2023

Uh oh!

fgvanzee commented Feb 13, 2023

Uh oh!

angsch commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fgvanzee commented Feb 10, 2023 •

edited

Loading

leekillough commented Feb 10, 2023 •

edited

Loading

fgvanzee commented Feb 10, 2023 •

edited

Loading

fgvanzee commented Feb 10, 2023 •

edited

Loading

fgvanzee commented Feb 11, 2023 •

edited

Loading