[PATCH] D64790: [LLVM][NFC] Adding an Alignment type to LLVM

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

In D64790#1588231 <https://reviews.llvm.org/D64790#1588231>, @jakehehrlich wrote:

>


First things first, thx a lot for the review. I really appreciate it.

> Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness.

Ok

> I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

Throughout LLVM we mostly use `unsigned` to represent an alignment. I don't know for sure what was the rationale for using it instead of uint64_t, it can be for historic reasons (just use the machine's `word` type) or for space reasons.
A quick estimation via `git grep -E "^\s+unsigned [Aa]lign.*" | wc -l` gives the following number of occurrences (for the whole `llvm-project` repo)

- `unsigned` 200
- `uint16_t`  4
- `uint32_t`  132
- `uint64_t`  44

One of the occurrences for the `uint16_t` is in `llvm/include/llvm/CodeGen/TargetLowering.h` so for this one at least is seems that storage is important.

This tends to prove that the Alignment type should be parameterized as you suggest.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64790/new/

https://reviews.llvm.org/D64790