[LangRef/DataLayout] Spell out requirements for alignment values #104705
Conversation
For 'p' the added wording matches the implementation. For 'i', 'f', 'v' the implementation also allows 0 for <pref> component, making 'i16:16:0' valid, for example. 'Fi0', 'Fn0' and 'S0' are also currently accepted. This is likely unintentional. There are no tests in the codebase that rely on this behavior, so the added wording prohibits zero alignments for these specifications. For 'a', the implementation currently allows 0 for both <abi> and <pref> components. The added wording prohibits specifying zero for <pref> with the same justification as above. Zero <abi> is used in tests, and the example at the end of the section suggests that this is valid, so that is left unchanged. This effectively prohibits zero alignments everywhere except for the <abi> component of aggregate specification.
|
@llvm/pr-subscribers-llvm-ir Author: Sergei Barannikov (s-barannikov) ChangesFor 'p' the added wording matches the implementation. For 'i', 'f', 'v' the implementation also allows 0 for <pref> component, making 'i16:16:0' valid, for example. 'Fi0', 'Fn0' and 'S0' are also currently accepted. This is likely unintentional. There are no tests in the codebase that rely on this behavior, so the added wording prohibits zero alignments for these specifications. For 'a', the implementation currently allows 0 for both <abi> and <pref> components. The added wording prohibits specifying zero for <pref> with the same justification as above. Zero <abi> is used in tests, and the example at the end of the section suggests that this is valid, so that's left unchanged. This effectively prohibits zero alignments everywhere except for the <abi> component of aggregate specification. Full diff: https://github.com/llvm/llvm-project/pull/104705.diff 1 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 5e5e9b9e8a93b1..ea4fb6030161aa 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -3030,8 +3030,10 @@ as follows:
``S<size>``
Specifies the natural alignment of the stack in bits. Alignment
promotion of stack variables is limited to the natural stack
- alignment to avoid dynamic stack realignment. The stack alignment
- must be a multiple of 8-bits. If omitted, the natural stack
+ alignment to avoid dynamic stack realignment.
+ The stack alignment must be must be in the range [1,2^16)
+ and must be a power of two times the byte width.
+ If omitted, the natural stack
alignment defaults to "unspecified", which does not prevent any
alignment promotions.
``P<address space>``
@@ -3065,16 +3067,22 @@ as follows:
are in bits. The address space, ``n``, is optional, and if not specified,
denotes the default address space 0. The value of ``n`` must be
in the range [1,2^24).
+ The values of ``<abi>`` and ``<pref>`` must be in the range [1,2^16)
+ and must be powers of two times the byte width.
``i<size>:<abi>[:<pref>]``
This specifies the alignment for an integer type of a given bit
``<size>``. The value of ``<size>`` must be in the range [1,2^24).
``<pref>`` is optional and defaults to ``<abi>``.
+ The values of ``<abi>`` and ``<pref>`` must be in the range [1,2^16)
+ and must be powers of two times the byte width.
For ``i8``, the ``<abi>`` value must equal 8,
that is, ``i8`` must be naturally aligned.
``v<size>:<abi>[:<pref>]``
This specifies the alignment for a vector type of a given bit
``<size>``. The value of ``<size>`` must be in the range [1,2^24).
``<pref>`` is optional and defaults to ``<abi>``.
+ The values of ``<abi>`` and ``<pref>`` must be in the range [1,2^16)
+ and must be powers of two times the byte width.
``f<size>:<abi>[:<pref>]``
This specifies the alignment for a floating-point type of a given bit
``<size>``. Only values of ``<size>`` that are supported by the target
@@ -3082,9 +3090,14 @@ as follows:
or 128 (different flavors of long double) are also supported on some
targets. The value of ``<size>`` must be in the range [1,2^24).
``<pref>`` is optional and defaults to ``<abi>``.
+ The values of ``<abi>`` and ``<pref>`` must be in the range [1,2^16)
+ and must be powers of two times the byte width.
``a:<abi>[:<pref>]``
This specifies the alignment for an object of aggregate type.
``<pref>`` is optional and defaults to ``<abi>``.
+ The values of ``<abi>`` and ``<pref>`` must be in the range [1,2^16)
+ and must be powers of two times the byte width.
+ The value of ``<<abi>>` may also be zero meaning one byte alignment.
``F<type><abi>``
This specifies the alignment for function pointers.
The options for ``<type>`` are:
@@ -3093,6 +3106,8 @@ as follows:
of functions, and is a multiple of ``<abi>``.
* ``n``: The alignment of function pointers is a multiple of the explicit
alignment specified on the function, and is a multiple of ``<abi>``.
+ The values of ``<abi>`` and ``<pref>`` must be in the range [1,2^16)
+ and must be powers of two times the byte width.
``m:<mangling>``
If present, specifies that llvm names are mangled in the output. Symbols
prefixed with the mangling escape character ``\01`` are passed through
|
llvm/docs/LangRef.rst
Outdated
| must be a multiple of 8-bits. If omitted, the natural stack | ||
| alignment to avoid dynamic stack realignment. | ||
| The stack alignment must be must be in the range [1,2^16) | ||
| and must be a power of two times the byte width. |
There was a problem hiding this comment.
I'm not very happy with this wording.
What I'm trying to say is that the value must be ByteWidth * 2^N ,without assuming that ByteWidth == 8.
|
I don't know who else I should add as reviewers. |
There was a problem hiding this comment.
Can we say once at the top that all alignment specs must be powers of two etc, rather than repeating it for each use?
There was a problem hiding this comment.
I did that because size and address spaces requirements are repeated too, but I don't feel like I should be moving them in this PR. (Or should I?)
There was a problem hiding this comment.
I've moved the repeated sentences to the bottom, next to the other text that talks about alignments.
llvm/docs/LangRef.rst
Outdated
| @@ -3030,8 +3030,7 @@ as follows: | |||
| ``S<size>`` | |||
| Specifies the natural alignment of the stack in bits. Alignment | |||
| promotion of stack variables is limited to the natural stack | |||
| alignment to avoid dynamic stack realignment. The stack alignment | |||
| must be a multiple of 8-bits. If omitted, the natural stack | |||
There was a problem hiding this comment.
It is implied in the added paragraph.
There was a problem hiding this comment.
This is an 8-bit per byte world and your downstream target differs from that assumption. Don't play games.
There was a problem hiding this comment.
Please be constructive in your criticism.
I'm marking this conversation as resolved unless you have any specific concerns.
There was a problem hiding this comment.
Not resolved. You substitute 8-bit by byte to remove the 8-bit restriction.
There was a problem hiding this comment.
I think it's fine omit the 8-bit mention here. It's only mentioned once for S and not any of the other alignment specifications. I looked through LangRef a bit, and with the notable exception of this sentence, the wording always tries to be byte-size agnostic.
There was a problem hiding this comment.
LangRef probably should mention somewhere that bytes are 8 bits large in LLVM -- this just isn't the right place, as it's a global assumption, not specific to only this one thing.
Not sure it makes sense to actually add that note now though, if there are plans to lift this restriction in the near future.
|
@nikic feature request: bytes are 8-bit. |
Looks like the data layouts emitted by clang even use |
llvm/docs/LangRef.rst
Outdated
| @@ -3123,6 +3120,9 @@ as follows: | |||
| as :ref:`Non-Integral Pointer Type <nointptrtype>` s. The ``0`` | |||
| address space cannot be specified as non-integral. | |||
|
|
|||
| Unless explicitly stated otherwise, on every specification that specifies | |||
| an alignment, the value of the alignment must be in the range [1,2^16) | |||
| and must be a power of two times the width of a byte. | |||
There was a problem hiding this comment.
| and must be a power of two times the width of a byte. | |
| and must be a power of two greater than the width of a byte. |
Would be slightly clearer imho -- but preclude non-power-of-two bytes. Was potentially allowing those in the future the motivation for the chosen wording?
There was a problem hiding this comment.
I'm only considering supporting 16-bit bytes as the most popular, and 32-bit as that is the width of a byte on my architecture.
For these cases either of the following will do:
- "must be a power of two and a multiple of the byte width"
- "must be a power of two and at least the width of a byte"
The suggested "must be a power of two times the width of a byte" is more relaxed as it allows any byte width, e.g., 24, so that 24-, 48- and 96-bit alignments are valid (1, 2 and 4 bytes, respectively) even though the values are not powers of two. I'm not pursuing this generalization; the idea was that if it's equally easy to read, why not allow it. (I don't find it easy to read at all, but I can only judge this by my limited English.)
There was a problem hiding this comment.
Okay, let's stick with your current wording :)
|
@nikic Are you still ok with this change? |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/199/builds/2625 Here is the relevant piece of the build log for the reference |
For 'p' the added wording matches the implementation.
For 'i', 'f', 'v' the implementation also allows 0 for
<pref>component, making 'i16:16:0' valid, for example. 'Fi0', 'Fn0' and 'S0' are also currently accepted. This is likely unintentional. There are no tests in the codebase that rely on this behavior, so the added wording prohibits zero alignments for these specifications.For 'a', the implementation currently allows 0 for both
<abi>and<pref>components. The added wording prohibits specifying zero for<pref>with the same justification as above. Zero<abi>is used in tests, and the example at the end of the section suggests that this is valid, so that's left unchanged.This effectively prohibits zero alignments everywhere except for the
<abi>component of aggregate specification.