Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Install helper headers to INCDIR prefix. #787

Merged
merged 4 commits into from
Nov 21, 2023
Merged

Install helper headers to INCDIR prefix. #787

merged 4 commits into from
Nov 21, 2023

Conversation

fgvanzee
Copy link
Member

@fgvanzee fgvanzee commented Nov 11, 2023 •
edited
Loading

Details:

  • Install one-line headers to INCDIR whose entire purpose is to #include the actual headers within the local blis header directory so that applications can #include "blis.h" instead of #include "blis/blis.h" (and/or "cblas.h" instead of "blis/cblas.h" if CBLAS is enabled) when headers are installed to global paths. (Note that INCDIR is the installation prefix for headers as specified by --includedir=INCDIR, which defaults to PREFIX/include if not specified.) Not sure how this problem went unreported for so long, since presumably any user trying to #include "blis.h" from a global installation would have encountered a compiler error.
  • The one-line blis.h and cblas.h headers now reside in the build directory, ready to install as is.
  • Thanks to to Jed Brown for reporting this via Issue Header path for default source build and Debian should match #786, and for Devin Matthews and Mo Zhou for their engagement.
  • Harmonized the rule in the top-level Makefile for installing blis.pc into SHAREDIR/pkgconfig with conventions for others vis-a-vis verbosity/non-verbosity.

cc: @jedbrown @cdluminate

@fgvanzee
Install helper headers to INCDIR prefix.
09ce0fc 
Details:
- Install one-line headers to INCDIR whose entire purpose is to
  #include the actual headers within the local 'blis' header directory
  so that applications can #include "blis.h" instead of #include
  "blis/blis.h" (and/or "cblas.h" instead of "blis/cblas.h" if CBLAS is
  enabled) when headers are installed to global paths. (Note that
  INCDIR is the installation prefix for headers as specified by
  '--includedir=INCDIR', which defaults to 'PREFIX/include' if not
  specified.) Not sure how this problem went unreported for so long,
  since presumably any user trying to #include "blis.h" from a global
  installation would have encountered a compiler error.
- The one-line blis.h and cblas.h headers now reside in the 'build'
  directory, ready to install as is.
- Thanks to to Jed Brown for reporting this via Issue #786, and for
  Devin Matthews and Mo Zhou for their engagement.
- Harmonized the rule in the top-level Makefile for installing blis.pc
  into SHAREDIR/pkgconfig with conventions for others vis-a-vis
  verbosity/non-verbosity.
@devinamatthews
Copy link
Member

Helper include files should use <> not "".

jedbrown
jedbrown reviewed Nov 11, 2023
View reviewed changes
Copy link
Contributor

@jedbrown jedbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many places throughout the repository that #include "blis.h" (especially user-facing docs) that should be #include <blis.h> because you don't actually want to search a relative path.

Makefile Outdated
@@ -282,7 +282,7 @@ endif
#

# Define a list of headers to install. The default is to only install blis.h.
HEADERS_TO_INSTALL := $(BLIS_H_FLAT)
HEADERS_TO_INSTALL := $(BLIS_H_FLAT)

# If CBLAS is enabled, we also install cblas.h so the user does not need to
# change their source code to #include "blis.h" in order to access the CBLAS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# change their source code to #include "blis.h" in order to access the CBLAS
# change their source code to #include <blis.h> in order to access the CBLAS

But this comment seems out of place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was likely a copy-paste bug. Thanks Jed, I'll fix it.

Copy link
Member Author

@fgvanzee fgvanzee Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked into this, and the mention of blis.h is actually not a typo. What it's trying to say is the following:

If the user already has an application that has #include "cblas.h" and wants to use BLIS as a CBLAS implementation, then we have to install the cblas.h header in order to provide the best portability experience. If we didn't install the cblas.h header, the user could still use CBLAS APIs via BLIS, but they'd have to update their application source code to use #include "blis.h" instead of #include "cblas.h".

I'll try to update the comment to clarify.

@fgvanzee
Copy link
Member Author

Helper include files should use <> not "".

Thanks. I'll change the contents of the helper headers to use < >.

@fgvanzee
Copy link
Member Author

Ugh, BSD install doesn't recognize -T. 💀

@fgvanzee
Copy link
Member Author

Actually, I may not need -T... 🤞🏼

@fgvanzee
Copy link
Member Author

@jedbrown @cdluminate If this looks good to you, I'll squash-merge, maybe later today.

@fgvanzee fgvanzee merged commit 141a6c9 into master Nov 21, 2023
2 checks passed
@fgvanzee fgvanzee deleted the helper_headers branch November 21, 2023 18:26
fgvanzee added a commit that referenced this pull request Dec 12, 2023
@fgvanzee
Install helper headers to INCDIR prefix. (#787)
8109d18 
Details:
- Install one-line headers to INCDIR whose entire purpose is to
  #include the actual headers within the local 'blis' header directory
  so that applications can #include "blis.h" instead of #include
  <blis/blis.h> (and/or "cblas.h" instead of <blis/cblas.h> if CBLAS is
  enabled) when headers are installed to global paths. (Note that
  INCDIR is the installation prefix for headers as specified by
  '--includedir=INCDIR', which defaults to 'PREFIX/include' if not
  specified.) Not sure how this problem went unreported for so long,
  since presumably any user trying to #include "blis.h" from a global
  installation would have encountered a compiler error.
- The one-line blis.h and cblas.h headers now reside in the 'build'
  directory, ready to install as is.
- Thanks to to Jed Brown for reporting this via Issue #786, and for
  Devin Matthews and Mo Zhou for their engagement.
- Harmonized the rule in the top-level Makefile for installing blis.pc
  into SHAREDIR/pkgconfig with conventions for others vis-a-vis
  verbosity/non-verbosity.
- (cherry picked from commit 141a6c9)
fgvanzee added a commit that referenced this pull request May 26, 2024
@fgvanzee
Fix incorrect commenting of BLIS_RNTM_INITIALIZER and
ed57f0f 
`BLIS_OBJECT_INITIALIZER`.
- (cherry picked from a316d2c)

Update BLIS_*_INITIALIZER macros for C++ compatibility. (#802)

Details:
- Remove designated initializer syntax. This isn't officially supported
  until C++20.
- Arrange initializers in the order in which they are defined in the
  struct. Even with standard or extension support for designated
  initializers, initializing non-static members out-of-order is an
  error in C++.
- Remove the conditional code which uses '-1' as the default value of
  the 'pack_buf' member of 'mem_t' in C, but 'BLIS_BUFFER_FOR_GEN_USE'
  in C++. Simply use the latter as a common-sense default.
- (cherry picked from 664cc6b)

Add cpu part codes for various manufacturers and use in the code (#794)

* Add cpu_id symbols for arm v8.

* Add symbols for arm v7.

* Always assume firestorm on Apple aarch64.

* Fixes incorrect usage of model vs. part in some places.

* Fixes #793
- (cherry picked from 1a8c818)

Fix errors and typos in docs/BLIS*API.md (#791)

Details:
- Fixed errors and unified formatting in docs/BLIS*API.md docs.
- (cherry picked from c382d8b)

Include bli_config.h before bli_system.h in cblas.h. (#789)

Details:
- Previously, in cblas.h, bli_config.h was being #included *after*
  bli_system.h, which meant that the BLIS_ENABLE_SYSTEM macro was
  never defined in time for proper OS detection. This bug only
  affected cblas.h -- blis.h had been correctly #including
  bli_config.h before bli_system.h since fb93d24. Thanks to
  Edward Smyth for reporting this bug and suggesting the fix.
- (cherry picked from a72e456)

Install helper headers to INCDIR prefix. (#787)

Details:
- Install one-line headers to INCDIR whose entire purpose is to
  #include the actual headers within the local 'blis' header directory
  so that applications can #include "blis.h" instead of #include
  <blis/blis.h> (and/or "cblas.h" instead of <blis/cblas.h> if CBLAS is
  enabled) when headers are installed to global paths. (Note that
  INCDIR is the installation prefix for headers as specified by
  '--includedir=INCDIR', which defaults to 'PREFIX/include' if not
  specified.) Not sure how this problem went unreported for so long,
  since presumably any user trying to #include "blis.h" from a global
  installation would have encountered a compiler error.
- The one-line blis.h and cblas.h headers now reside in the 'build'
  directory, ready to install as is.
- Thanks to to Jed Brown for reporting this via Issue #786, and for
  Devin Matthews and Mo Zhou for their engagement.
- Harmonized the rule in the top-level Makefile for installing blis.pc
  into SHAREDIR/pkgconfig with conventions for others vis-a-vis
  verbosity/non-verbosity.
- (cherry picked from 141a6c9)

Allow users to defines [sd]complex using std::complex (#784)

Details:
- In C++ applications, it makes a lot of sense to interface to BLIS
  using C++'s standard complex number library, which uses a template
  class std::complex. Obviously BLIS doesn't know anything about this
  and defaults to a custom struct to represent complex numbers. This PR
  updates the bli_[cz]{real,imag}() functions to accept std::complex
  numbers when a C++ compiler is being used. Note that this has no
  effect on the compilation of the BLIS library (or testsuite), and only
  comes into play when including blis.h into a C++ project and forcing
  the use of std::complex for scomplex and dcomplex.
- The application can explicitly request std:complex-based types via:

    #define BLIS_ENABLE_STD_COMPLEX
    #include <blis.h>
    // Call BLIS functions using std::complex<double> here.

- Fixed a bug in the definition of some scalar level-0 macros, since
  bli_creal()/bli_cimag() and bli_zreal()/bli_zimag() are no longer
  interchangeable.
- (cherry picked from 2d94392)

CREDITS file update.

- (cherry picked from f7ce54a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedbrown jedbrown jedbrown left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fgvanzee @devinamatthews @jedbrown