[llvm-dev] Introducing an Alignment object in LLVM

Fri Jul 12 14:23:21 PDT 2019

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>