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.

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

Replace alignment_of_long_double with C11 alignof #12658

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devreal
Copy link
Contributor

@devreal devreal commented Jul 8, 2024

This removes a static variable in a static function and uses C11 provides alignof instead to get a compile-time value.

Also fixes this compiler warning:

opal_copy_functions_heterogeneous.c:495:1: warning: function declaration isn't a prototype [-Wstrict-prototypes]
  495 | alignment_of_long_double() {
      | ^~~~~~~~~~~~~~~~~~~~~~~~

I'm not sure why alignof would not be portable, since it's part of the C11 standard. All C11 compilers I tried on godbolt worked as expected (and some exotic 32bit compiler versions seemed to indicate 8B alignment for long double, not 4). Asking the compiler for the alignment seems to be the safe and reasonable choice.

@wenduwan
Copy link
Contributor

wenduwan commented Jul 8, 2024

Running AWS CI

@bosilca
Copy link
Member

bosilca commented Jul 8, 2024

We're not supporting 32 bits anymore, so moving to alignof is sensible.

@rhc54
Copy link
Contributor

rhc54 commented Jul 8, 2024

Errr...just a question. Is OMPI now requiring C11 compiler? We historically chose to require C11 atomics, but not a C11 compiler. Just wondering if you are adding a new requirement as PMIx and PRRTE don't.

@devreal
Copy link
Contributor Author

devreal commented Jul 8, 2024

My understanding was that we use C11 (not just the atomics). I might be wrong though. It's been 12+ years though, so maybe it's it's time to move on from last century?

@bosilca
Copy link
Member

bosilca commented Jul 8, 2024

The C11 operator is _Alignof, and the C23 operator is alignof according to https://en.cppreference.com/w/c/language/_Alignof

@devreal
Copy link
Contributor Author

devreal commented Jul 8, 2024

The C11 operator is _Alignof, and the C23 operator is alignof according to https://en.cppreference.com/w/c/language/_Alignof

Right, C11 provided alignof as a macro while C23 made it an operator. See https://en.cppreference.com/w/c/types for the macro.

@rhc54
Copy link
Contributor

rhc54 commented Jul 8, 2024

My understanding was that we use C11 (not just the atomics). I might be wrong though. It's been 12+ years though, so maybe it's it's time to move on from last century?

I honestly don't remember, except that this was a significant discussion (separating the two) at the time when the C11 atomic requirement was proposed. We intentionally left the base level of the C compiler lower because of defaults in distros. May well be that has changed - truly don't know. Just wanted to point it out here as we have unintentionally broken things in the past with what appeared to be an innocent/reasonable change.

Might be worth checking to see what OMPI actually does require today, and then discuss what that requirement should be going forward. FWIW: we required C99 for many, many years beyond what seemed "reasonable" just because of long-running distro defaults.

@devreal
Copy link
Contributor Author

devreal commented Jul 8, 2024

It looks like we're trying to enable C11 across the board (if the compiler needs a specific flag) but it is not an error if C11 support is not detected: https://github.com/open-mpi/ompi/blob/main/config/opal_setup_cc.m4#L182

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

After some discussion on the RM call today (let's also discuss on community call tomorrow):

  • Let's force the use of C11 for v6.0.x (i.e., doing this on main is fine -- but let's not bring it to v5.0.x)
  • Let's also update the m4 to enforce that we need C11 (per what @devreal cited)
  • Let's also update the docs to state that C11 is a prereq (wherever we currently state C99)

@devreal devreal force-pushed the alignof-long-double branch 2 times, most recently from 4d26b40 to 48c2dd8 Compare July 9, 2024 18:30
This removes a static variable in a static function and uses
C11 `alignof` instead to get a compile-time value. Since we don't
currently require C11 we keep the old implementation as fallback.

Also fixes this compiler warning:

```
opal_copy_functions_heterogeneous.c:495:1: warning: function declaration isn't a prototype [-Wstrict-prototypes]
  495 | alignment_of_long_double() {
      | ^~~~~~~~~~~~~~~~~~~~~~~~
```
by adding `void` as parameter.

Signed-off-by: Joseph Schuchart <[email protected]>
@janjust
Copy link
Contributor

janjust commented Oct 8, 2024

depends on #12660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants