[llvm-dev] Introducing an Alignment object in LLVM
Guillaume Chatelet via llvm-dev
llvm-dev at lists.llvm.org
Fri Jul 12 02:20:37 PDT 2019
Alignment in LLVM is currently represented with an `unsigned`, sometimes an
`uint32_t`, `uint64_t` or `uint16_t`.
FWIU the value has the following possible semantics:
- 0 means alignment is unknown,
- 1 means no alignment requirement,
- a power of two means a required alignment in bytes.
Using `unsigned` throughout the codebase has several disadvantages:
- comparing alignments may compare different things, what does `A < B` or
`std::min(A, B)` mean when A or B are allowed to be 0?
- integer promotion can kick in and turn a `bool` in an alignment when
calling a function (1)
- masking may lead to incorrect result when value is 0 (2)
- integer promotion leads to not obviously correct code in the presence of
signed and unsigned values (3)
- dividing an alignment by 2 can change the associated semantic from
`unaligned` to `unknown alignment` (4)
- developers have to be defensive to make sure assumptions hold (5)
- checking that an offset is aligned is sometimes done backward `Alignment
% Offset == 0` instead of `Offset % Alignment == 0` (6) (7)
- MachineConstantPoolEntry::Alignment encodes special information in its
topmost bit (8) but `AsmPrinter::GetCPISymbol` seems to use it directly (9)
instead of calling `getAlignment()` (10)
I have a patch to introduce alignment object in LLVM.
This patch does not fix the code but replaces the unsigned value by a type
so it's easier to introduce proper semantic later on.
The patch (11) is too big to be sent to Phabricator, arc diff complains
about server's `post_max_size` being too small.
I would like to seek guidance from the community:
- Is this patch worthwhile?
- If so, how should it be reviewed? Should it be split?
PS: If you intend to have a look at it you should start with
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-dev