llvm dev - Jul 2019 - Introducing an Alignment object in LLVM

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?

-- Guillaume
PS: If you intend to have a look at it you should start with
`llvm/include/llvm/Support/Alignment.h`

1 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp#L2593
2 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/SafeStack.cpp#L545
3 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/Hexagon/HexagonFrameLowering.cpp#L1533
4 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6751
5 -
https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/include/llvm/Analysis/VectorUtils.h#L278
6 -
https://github.com/llvm/llvm-project/blob/fafec5155e39f5dad098376c1beb4a56604aa655/llvm/lib/Target/ARM/ARMCallingConv.cpp#L207
7 -
https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp#L563
8 -
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L76
9 -
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L2809
10 -
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L96
11 -
https://drive.google.com/file/d/1PgrEuNjIcSH9hOs6REe9FedCH5mf43Q9/view?usp=sharing
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190712/cb309c24/attachment.html>

Without looking at the patch, I like this idea. Numeric quantities in general
(alignment, bits, bytes, etc) are often confusing in the codebase IMO, and what
you propose would help alleviate this problem.

Can you fix this incrementally? i.e. add an alignment class which has implicit
conversions to the current unsigned convention, and incrementally replace it
throughout the codebase. Once everything is fixed, remove the implicit
conversions.

> On Jul 12, 2019, at 2:20 AM, Guillaume Chatelet via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> 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?
> 
> -- Guillaume
> PS: If you intend to have a look at it you should start with
`llvm/include/llvm/Support/Alignment.h`
> 
> 1 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp#L2593
<https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp#L2593>
> 2 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/SafeStack.cpp#L545
<https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/SafeStack.cpp#L545>
> 3 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/Hexagon/HexagonFrameLowering.cpp#L1533
<https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/Hexagon/HexagonFrameLowering.cpp#L1533>
> 4 -
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6751
<https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6751>
> 5 -
https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/include/llvm/Analysis/VectorUtils.h#L278
<https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/include/llvm/Analysis/VectorUtils.h#L278>
> 6 -
https://github.com/llvm/llvm-project/blob/fafec5155e39f5dad098376c1beb4a56604aa655/llvm/lib/Target/ARM/ARMCallingConv.cpp#L207
<https://github.com/llvm/llvm-project/blob/fafec5155e39f5dad098376c1beb4a56604aa655/llvm/lib/Target/ARM/ARMCallingConv.cpp#L207>
> 7 -
https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp#L563
<https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp#L563>
> 8 -
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L76
<https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L76>
> 9 -
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L2809
<https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L2809>
> 10 -
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L96
<https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L96>
> 11 -
https://drive.google.com/file/d/1PgrEuNjIcSH9hOs6REe9FedCH5mf43Q9/view?usp=sharing
<https://drive.google.com/file/d/1PgrEuNjIcSH9hOs6REe9FedCH5mf43Q9/view?usp=sharing>_______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190712/527fcb1c/attachment.html>

Woah this is a good idea.

I'd ask that alignment come in different bit sizes and endienesses so that
we can add an alignment type to ELF types. I would love to review this and
add it to llvm-objcopy. We have special functions to handle all of these
'zero' cases. Several other bits of code I've seen/written have to
find
maximum alignment and I'd imagine the mistake of not accounting for zero is
common.

Where's the patch? Add me as a reviewer and I'll look at it today or
Monday.

On Fri, Jul 12, 2019 at 9:07 AM JF Bastien via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Without looking at the patch, I like this idea. Numeric quantities in
> general (alignment, bits, bytes, etc) are often confusing in the codebase
> IMO, and what you propose would help alleviate this problem.
>
> Can you fix this incrementally? i.e. add an alignment class which has
> implicit conversions to the current unsigned convention, and incrementally
> replace it throughout the codebase. Once everything is fixed, remove the
> implicit conversions.
>
>
> On Jul 12, 2019, at 2:20 AM, Guillaume Chatelet via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> 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?
>
> -- Guillaume
> PS: If you intend to have a look at it you should start with
> `llvm/include/llvm/Support/Alignment.h`
>
> 1 -
>
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp#L2593
> 2 -
>
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/SafeStack.cpp#L545
> 3 -
>
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/Target/Hexagon/HexagonFrameLowering.cpp#L1533
> 4 -
>
https://github.com/llvm/llvm-project/blob/b725d27350ffdda9e8e9144668ad54c922297f59/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6751
> 5 -
>
https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/include/llvm/Analysis/VectorUtils.h#L278
> 6 -
>
https://github.com/llvm/llvm-project/blob/fafec5155e39f5dad098376c1beb4a56604aa655/llvm/lib/Target/ARM/ARMCallingConv.cpp#L207
> 7 -
>
https://github.com/llvm/llvm-project/blob/d0307f93a7658e6d0eef1ffd0b0ed4f1506bfc13/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp#L563
> 8 -
>
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L76
> 9 -
>
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L2809
> 10 -
>
https://github.com/llvm/llvm-project/blob/7eeb82b58554163962d2696ce9be7d021d5b25d4/llvm/include/llvm/CodeGen/MachineConstantPool.h#L96
> 11 -
>
https://drive.google.com/file/d/1PgrEuNjIcSH9hOs6REe9FedCH5mf43Q9/view?usp=sharing
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190712/484d0276/attachment.html>