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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

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

> In D64790#1588982 <https://reviews.llvm.org/D64790#1588982>, @gchatelet wrote:
>
> > 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.
>
>
> In those 44 cases and 132 cases includes all ELF code. Also consider lld for more uses. You alienate no one if you support up to 64-bits but you alienate all ELF programmers if you don't support 64-bits. I am one of those people. I cannot use this if 64-bits isn't supported. Other than trying to help llvm code quality improve and being a good citizen I have no reason to review this if I don't get 64-bit support.


Please don't get me wrong, I'm absolutely willing to support 64-bits. Here I'm trying to see what's the best design since there are different tradeoffs:

- Have a single 64-bits type: accommodates all usage but can be a problem where space matters
- Have a templated type so people can use whatever suites them best, this complicates the code though.
- Have a compressed representation in a single byte but paying an extra cost when accessing it.

I don't think I ever said I would not add 64-bits support, if it looks like so I apologize for the misunderstanding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64790