[llvm-dev] Introducing an Alignment object in LLVM

Wed Jul 31 10:03:18 PDT 2019

For the record, the type is now in: https://reviews.llvm.org/D64790.
I'll follow up with a bunch of patches and incrementally change the code
base to use the new type.

   - https://reviews.llvm.org/D65514
   - https://reviews.llvm.org/D65521

Thx a lot to jfb and jakehehrlich for the review.

On Mon, Jul 15, 2019 at 10:09 AM Guillaume Chatelet <gchatelet at google.com>
wrote:

> @JF Bastien : Indeed I think incremental fixing is the way to go - not my
> preferred option but the only feasible one considering the size of the
> patch.
> I'll start by introducing the Alignment object and its unittests and we
> can start the discussion from here.
>
> @Jake Ehrlich : Can you point me to source code where endianness matters?
> I never encountered it when I refactored the code.
> Also I understand why different bit sizes can help but I'm not convinced
> it's worth the additional complexity (conversion, assignment, construction,
> comparisons between different bit sizes).
> `uint32_t` seems to be a good fit for now, we can start with this, do the
> refactoring and introduce a templated type afterwards if it works for you?
>
> On Fri, Jul 12, 2019 at 11:23 PM Jake Ehrlich <jakehehrlich at google.com>
> wrote:
>
>> 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/20190731/0fa2e629/attachment.html>