Introducing an Alignment object in LLVM

Project Infrastructure LLVM Dev List Archives
1

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

2

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.

3

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.

4

@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?

5

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.