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

Simplify CAMLalign and use C11 max_align_t #13139

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Apr 30, 2024

Some cleanups removing checks and workarounds for older compilers, assuming that the compiler supports C11 or C++11 out of the box. We may use _Alignas (since C11) or alignas (since C23) directly, and use the max_align_t type. Unfortunately, support for max_align_t is missing from the Windows C standard library.

In our public headers, we're using either:
- C23 where `alignas` is a keyword;
- C++11 or later where `alignas` is also available;
- C11/C17 where `_Alignas` is available.
For C++, MSVC defines `using max_align_t = double`. Take inspiration
from GCC and LLVM for the fallback implementation.

It's unlikely that we need to carry a fallback implementation for
other compilers. If so, the following could be used:

    #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L || \
        defined(__cplusplus)
    #define CAMLalignof(n) alignof(n)
    #else
    #define CAMLalignof(n) _Alignof(n)
    #endif

    typedef struct {
      long long ll CAMLalign(CAMLalignof(long long));
      long double ld CAMLalign(CAMLalignof(long double));
    } max_align_t;

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/ginclude/stddef.h
https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/__stddef_max_align_t.h

https://en.cppreference.com/w/c/types/max_align_t
@MisterDA MisterDA force-pushed the alignas-simplify-CAMLalign branch from a49b31b to d226d67 Compare May 21, 2024 06:48
Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

Looks good to me. +1 for using _Alignas and alignas in preference to attributes. One suggestion below concerning the MSVC fallback.


struct pool_block {
#ifdef DEBUG
intnat magic;
#endif
struct pool_block *next;
struct pool_block *prev;
union max_align data[]; /* not allocated, used for alignment purposes */
max_align_t data[]; /* not allocated, used for alignment purposes */
};
Copy link
Contributor

Choose a reason for hiding this comment

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

One downside with this use of double as fallback max_align_t type is that double has rather low alignment: 8 on x86_64, 4 on x86_32, while most SSE vector instructions require 16-alignment. (In Linux x86_64 and macOS x86_64, max_align_t has 16 alignment.) What about using an explicit 16 alignment as the fallback case?

struct pool_block {
#ifdef DEBUG
  intnat magic;
#endif
  struct pool_block *next;
  struct pool_block *prev;
#ifdef HAVE_MAX_ALIGN_T
  max_align_t data[]; /* not allocated, used for alignment purposes */
#else
  CAMLalign(16) char data[];  /* 16 is a reasonable alignment default */
#endif
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd tend to align (ha, ha) with your suggestion, but: MSVC C++ cstddef.h header uses double alignment for max_align_t, and clang-cl uses double too, which I think makes it a reasonable default for Windows.
As for a general fallback, I hope that other compilers+libc aren't as buggy, and I wonder if we do need to provide a definition, or rather catch a missing definition as a compilation failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

MSVC C++ cstddef.h header uses double alignment for max_align_t

It is in error, then. The program below, compiled with MSVC, prints 16, showing that there are types with alignment greater than 8.

#include <iostream>
#include <xmmintrin.h>

int main() {
    std::cout << alignof(__m128) << '\n';
    return 0;
}

In this PR, you're not trying to emulate whatever strange choices MSVC does, but to make sure OCaml's pool allocator works as intended. If the intent is to align for the maximal alignment constraint of the target platform, the alignment must be >= 16 on x86, because SSE instructions. If the intent is to align for the biggest datatype OCaml stores in heap blocks, word-alignment is enough and you don't need to add anything to struct pool_block to guarantee it, as it already contains two word-sized pointers.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about defaulting to long double rather than double then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is in error, then. The program below, compiled with MSVC, prints 16, showing that there are types with alignment greater than 8.

At least for C++, I don't think there's an error.

The type max_align_t is a POD type whose alignment requirement is at least as great as that of every scalar type, and whose alignment requirement is supported in every context. [C++11]

#include <iostream>
#include <xmmintrin.h>
#include <cstddef>
#include <type_traits>

int main() {
    std::cout << alignof(__m128) << std::endl
              << alignof(std::max_align_t) << std::endl
              << std::is_scalar<__m128>() << std::endl;
    return 0;
}

shows 16, 8, 0, indicating that __m128 isn't considered a scalar type, and thus the definition of std::max_align_t is consistent (with respect to the __m128 type).

The definition for C is more vague and I guess it could be argued that 16 would be a correct value.

max_align_t which is an object type whose alignment is as great as is supported by the implementation in all contexts; [C11]

What about defaulting to long double rather than double then?

long double is identical to double under MSVC 1.

Thanks for the thorough review. I think the real question is indeed whether the intent is to align for the maximal alignment constraint of the target platform, or to align for the biggest datatype OCaml stores in heap blocks. None of the fields in the former union max_align had a 16 bytes alignment, and all worked well, hasn't it? Now it's not clear to me that this field is actually needed.

Footnotes

  1. https://learn.microsoft.com/en-us/cpp/c-language/type-long-double?view=msvc-170

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.

None yet

3 participants