Skip to content

Omit -fPIC if shared library build is disabled.#732

Merged
fgvanzee merged 1 commit intomasterfrom
fpic_fix
Mar 25, 2023
Merged

Omit -fPIC if shared library build is disabled.#732
fgvanzee merged 1 commit intomasterfrom
fpic_fix

Conversation

@fgvanzee
Copy link
Copy Markdown
Member

@fgvanzee fgvanzee commented Mar 25, 2023

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 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 being built. Thanks to Nick Knight for reporting this issue.
  • CREDITS file update.

cc: @nick-knight

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 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 being
  built. Thanks to Nick Knight for reporting this issue.
- CREDITS file update.
@fgvanzee fgvanzee merged commit 5f84130 into master Mar 25, 2023
@fgvanzee fgvanzee deleted the fpic_fix branch March 25, 2023 01:05
@nick-knight
Copy link
Copy Markdown

Hi @fgvanzee , I think I may have misunderstood the intention here.

The documentation says:

CPICFLAGS should be assigned flags to enable position independent code (shared library) flags.

This suggested to me that CPICFLAGS are the things I want passed to the compiler in the case of --enable-shared, but not in the case of --disable-shared. It appears to me --- and I could be mistaken --- that something different is happening:

  • if --enable-shared, our custom CPICFLAGS are being overridden in common.mk;
  • if --disable-shared, our custom CPICFLAGS are being passed.

At this point, I'm not sure I fully understand the role of CPICFLAGS in make_defs.mk.

@fgvanzee
Copy link
Copy Markdown
Member Author

@nick-knight Good catch. I think this was a mistake on our part (or a somewhat sloppy hack). I think I agree that the -fPIC flag should be applied within the make_defs.mk of each subconfig -- at least, I can't see any reason why it should be otherwise. And if that were the case, common.mk doesn't need to override or append any flags (although it appears in the case of Windows builds, -fPIC needs to be removed).

I can submit a new PR that moves the -fPIC flag insertion to the make_defs.mk files.

@fgvanzee
Copy link
Copy Markdown
Member Author

fgvanzee commented Mar 29, 2023

@nick-knight I now realize that common.mk was doing something slightly less bizarre, which was appending the -fPIC flag to whatever the subconfigs set within their individual make_defs.mk files (although for Windows, the appending behavior was not doing what it was supposed to, and was only working because the subconfigs' CPICFLAGS variables were all blank! 😂 ).

At this point, I don't think common.mk has any business appending anything to the CPICFLAGS variables, so I'm going to go ahead with the separate PR.

ct-clmsn pushed a commit to ct-clmsn/blis that referenced this pull request Jul 29, 2023
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.
fgvanzee added a commit that referenced this pull request May 21, 2024
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)
fgvanzee added a commit that referenced this pull request May 22, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants