Export functions without def file by isuruf · Pull Request #303 · flame/blis

isuruf · 2019-02-27T23:35:29Z

@fgvanzee, can you confirm that the functions removed from libblis-symbols.def file are not intended to be public API?

Fixes #248

This reverts commit 09fb07c.

fgvanzee · 2019-02-27T23:42:14Z

@fgvanzee, can you confirm that the functions removed from libblis-symbols.def file are not intended to be public API?

@isuruf Yes, I will check. Thanks.

fgvanzee · 2019-02-27T23:52:16Z

@isuruf The functions you removed from libblis-symbols.def all appear to be internal/helper functions--no public functions that I could see.

isuruf · 2019-02-28T00:41:25Z

Ready for review.

CFLAGS="-fvisibility=hidden" ./configure auto should remove those few internal functions from being exported now.

After this PR is merged, BLIS_EXPORT_BLIS macro should be removed from any internal API.

fgvanzee · 2019-02-28T00:56:35Z

Ready for review.

Will do. Thanks Isuru.

CFLAGS="-fvisibility=hidden" ./configure auto should remove those few internal functions from being exported now.

I think you are suggesting that we can now adjust the Makefile so that -fvisibility=hidden is always used? Please confirm.

Also, do you know if that compiler option is recognized by all of: gcc, clang, and icc? If we need to use a different option for clang or icc (or both), that's fine. I just need to (eventually) know what the equivalent option(s) is/are.

After this PR is merged, BLIS_EXPORT_BLIS macro should be removed from any internal API.

Got it. I'll add this to my near-term to-do list, though I have a lot on my plate in the next couple days, so it might not be until early next week that I can make significant progress on this.

Also, if you don't mind, I'll wait on merging this PR until I can remove the macros from the internal APIs (I'll push to the PR branch). That way, you can also easily review/comment on it before I merge.

isuruf · 2019-02-28T01:30:06Z

I think you are suggesting that we can now adjust the Makefile so that -fvisibility=hidden is always used? Please confirm.

A user can easily pass the -fvisibility=hidden option or you can set it in the configure.
I'll leave that up to you to decide what to do.

Also, do you know if that compiler option recognized by all of: gcc, clang, and icc? If we need to use a different option for clang or icc (or both), that's fine. I just need to (eventually) know what the equivalent option(s) is/are.

I know that gcc and clang on unix do. (clang doesn't on windows). Not sure about icc.

Also, if you don't mind, I'll wait on merging this PR until I can remove the macros from the internal APIs (I'll push to the PR branch). That way, you can also easily review/comment on it before I merge.

Sure. I'm not in a hurry.

fgvanzee · 2019-02-28T01:33:43Z

A user can easily pass the -fvisibility=hidden option or you can set it in the configure.
I'll leave that up to you to decide what to do.

Understood. I'll probably make it a configure option eventually. But reducing it down to that flag's presence or absence is a great improvement. Thanks again for your efforts.

fgvanzee · 2019-03-08T18:25:50Z

@isuruf I was taking a closer look at the use of BLIS_EXPORT_BLIS. It seems that you mostly targeted function prototypes. However, the macro was also inserted into a few function definitions.

Can you briefly explain which one, prototypes or definitions, the macro must be used in front of in order to have the desired effect vis-a-vis hiding/exposing symbols in shared libraries?

isuruf · 2019-03-08T18:29:31Z

From prototypes I assume you mean function declaration. The macro should be added to the declaration and not the definition. If you can point out the definitions I have used the macro on, I'll fix them

fgvanzee · 2019-03-08T18:39:27Z

@isuruf Yes, I mean declaration. Not sure which term is in vogue these days; I call them prototypes since that was what they were called when I learned C. :)

You can find the affected source files by executing grep -R BLIS_EXPORT_BLIS frame | sort | grep "\.c:" | less at the top-level directory.

It may also be good to do the same thing but for BLIS_EXPORT_BLAS.

fgvanzee · 2019-03-11T20:07:44Z

@isuruf Your removal of BLIS_EXPORT_BLIS/BLIS_EXPORT_BLAS from function definitions looks good. I also appreciate that you did not leave behind the space after the macro! :)

fgvanzee · 2019-03-12T16:29:02Z

@isuruf I am now going to go through all of the remaining occurrences of BLIS_EXPORT_BLIS and remove the ones associated with functions that should always be private/internal.

isuruf · 2019-03-12T17:02:49Z

Thanks @fgvanzee

fgvanzee · 2019-03-12T23:26:47Z

Interesting observation: Because @isuruf wrote "Fixes #248" in the initial issue posting, merging this PR automatically closed #248. That seems like a generally smart and useful feature in GitHub, though not necessarily for those of us who start out oblivious to the effects of those magic words. :) It took me a little bit of digging to figure out what had happened.

Did you know about this, Isuru?

Details: - After merging PR #303, at Isuru's request, I removed the use of BLIS_EXPORT_BLIS from all function prototypes *except* those that we potentially wish to be exported in shared/dynamic libraries. In other words, I removed the use of BLIS_EXPORT_BLIS from all prototypes of functions that can be considered private or for internal use only. This is likely the last big modification along the path towards implementing the functionality spelled out in issue #248. Thanks again to Isuru Fernando for his initial efforts of sprinkling the export macros throughout BLIS, which made removing them where necessary relatively painless. Also, I'd like to thank Tony Kelman, Nathaniel Smith, Ian Henriksen, Marat Dukhan, and Matthew Brett for participating in the initial discussion in issue #37 that was later summarized and restated in issue #248. - CREDITS file update.

fgvanzee · 2019-03-12T23:49:24Z

@isuruf I am now going to go through all of the remaining occurrences of BLIS_EXPORT_BLIS and remove the ones associated with functions that should always be private/internal.

@isuruf Done. Take a look at 5a5f494 for the finished product. (Everything else is merged back to master, now, too.)

isuruf · 2019-03-13T01:19:34Z

Thanks. 5a5f494 looks good to me. Maybe something in docs asking the user to add -fvisibility=hidden to hide private API would help.

Yes, the fixes word was intentional. Closes and resolves also work.

fgvanzee · 2019-03-13T01:53:50Z

Thanks for those pro-tips. I'll work on something for the docs. Or, perhaps we should open a new issue to discuss whether -fvisibility=hidden should be the default for building shared libraries of BLIS. (I am open to it.) Maybe a new configure option would help, too.

* Revert "restore bli_extern_defs exporting for now" This reverts commit 09fb07c. * Remove symbols not intended to be public * No need of def file anymore * Fix whitespace * No need of configure option * Remove export macro from definitions * Remove blas export macro from definitions

Details: - After merging PR flame#303, at Isuru's request, I removed the use of BLIS_EXPORT_BLIS from all function prototypes *except* those that we potentially wish to be exported in shared/dynamic libraries. In other words, I removed the use of BLIS_EXPORT_BLIS from all prototypes of functions that can be considered private or for internal use only. This is likely the last big modification along the path towards implementing the functionality spelled out in issue flame#248. Thanks again to Isuru Fernando for his initial efforts of sprinkling the export macros throughout BLIS, which made removing them where necessary relatively painless. Also, I'd like to thank Tony Kelman, Nathaniel Smith, Ian Henriksen, Marat Dukhan, and Matthew Brett for participating in the initial discussion in issue #37 that was later summarized and restated in issue flame#248. - CREDITS file update.

* Revert "restore bli_extern_defs exporting for now" This reverts commit 09fb07c. * Remove symbols not intended to be public * No need of def file anymore * Fix whitespace * No need of configure option * Remove export macro from definitions * Remove blas export macro from definitions

Details: - After merging PR flame#303, at Isuru's request, I removed the use of BLIS_EXPORT_BLIS from all function prototypes *except* those that we potentially wish to be exported in shared/dynamic libraries. In other words, I removed the use of BLIS_EXPORT_BLIS from all prototypes of functions that can be considered private or for internal use only. This is likely the last big modification along the path towards implementing the functionality spelled out in issue flame#248. Thanks again to Isuru Fernando for his initial efforts of sprinkling the export macros throughout BLIS, which made removing them where necessary relatively painless. Also, I'd like to thank Tony Kelman, Nathaniel Smith, Ian Henriksen, Marat Dukhan, and Matthew Brett for participating in the initial discussion in issue #37 that was later summarized and restated in issue flame#248. - CREDITS file update.

* Revert "restore bli_extern_defs exporting for now" This reverts commit 09fb07c. * Remove symbols not intended to be public * No need of def file anymore * Fix whitespace * No need of configure option * Remove export macro from definitions * Remove blas export macro from definitions

Details: - After merging PR flame#303, at Isuru's request, I removed the use of BLIS_EXPORT_BLIS from all function prototypes *except* those that we potentially wish to be exported in shared/dynamic libraries. In other words, I removed the use of BLIS_EXPORT_BLIS from all prototypes of functions that can be considered private or for internal use only. This is likely the last big modification along the path towards implementing the functionality spelled out in issue flame#248. Thanks again to Isuru Fernando for his initial efforts of sprinkling the export macros throughout BLIS, which made removing them where necessary relatively painless. Also, I'd like to thank Tony Kelman, Nathaniel Smith, Ian Henriksen, Marat Dukhan, and Matthew Brett for participating in the initial discussion in issue #37 that was later summarized and restated in issue flame#248. - CREDITS file update.

…lame#303) - The current build systems have the following behaviour with regards to building "aocl_gemm" addon codebase(LPGEMM) when giving "amdzen" as the target architecture(fat-binary) - Make: Attempts to compile LPGEMM kernels using the same compiler flags that the makefile fragments set for BLIS kernels, based on the compiler version. - CMake: With presets, it always enables the addon compilation unless explicitly specified with the ENABLE_ADDON variable. - This poses a bug with older compilers, owing to them not supporting BF16 or INT8 intrinsic compilation. - This patch adds the functionality to check for GCC and Clang compiler versions, and disables LPGEMM compilation if GCC < 11.2 or Clang < 12.0. - Make: Updated the configure script to check for the compiler version if the addon is specified. CMake: Updated the main CMakeLists.txt to check for the compiler version if the addon is specified, and to also force-update the associated cache variable update. Also updated kernels/CMakeLists.txt to check if "aocl_gemm" remains in the ENABLE_ADDONS list after all the checks in the previous layers. AMD-Internal: [CPUPL-7850] Signed-off by : Vignesh Balasubramanian <[email protected]>

isuruf added 4 commits February 27, 2019 17:31

Revert "restore bli_extern_defs exporting for now"

11b0523

This reverts commit 09fb07c.

Remove symbols not intended to be public

7c6df52

No need of def file anymore

509df8f

Fix whitespace

358d447

isuruf mentioned this pull request Feb 27, 2019

add version script for BLAS/CBLAS-only purposes #301

Closed

No need of configure option

c6f9f37

isuruf added 2 commits March 8, 2019 22:54

Remove export macro from definitions

7d17c8a

Remove blas export macro from definitions

892e531

fgvanzee merged commit 766769e into flame:master Mar 12, 2019

isuruf deleted the export branch March 12, 2019 17:02

This was referenced Mar 12, 2019

Use -fvisibility=hidden and explicitly mark symbols for shared object export #248

Closed

Enable shared library build on Travis CI #37

Closed

fgvanzee mentioned this pull request Mar 13, 2019

Default visibility of shared library symbols #304

Closed

Conversation

isuruf commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgvanzee commented Feb 27, 2019

Uh oh!

fgvanzee commented Feb 27, 2019

Uh oh!

isuruf commented Feb 28, 2019

Uh oh!

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

Uh oh!

Uh oh!

isuruf commented Feb 28, 2019

Uh oh!

fgvanzee commented Feb 28, 2019

Uh oh!

fgvanzee commented Mar 8, 2019

Uh oh!

isuruf commented Mar 8, 2019

Uh oh!

fgvanzee commented Mar 8, 2019

Uh oh!

fgvanzee commented Mar 11, 2019

Uh oh!

fgvanzee commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isuruf commented Mar 12, 2019

Uh oh!

fgvanzee commented Mar 12, 2019

Uh oh!

fgvanzee commented Mar 12, 2019

Uh oh!

isuruf commented Mar 13, 2019

Uh oh!

fgvanzee commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

isuruf commented Feb 27, 2019 •

edited

Loading

fgvanzee commented Feb 28, 2019 •

edited

Loading

fgvanzee commented Mar 12, 2019 •

edited

Loading