[PATCH] D64790: [LLVM][NFC] Adding an Alignment type to LLVM
Guillaume Chatelet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 29 07:37:24 PDT 2019
gchatelet added a comment.
Thx for the review @jfb. Anything else?
================
Comment at: llvm/include/llvm/Support/Alignment.h:21
+// `undefined alignment requirements` and 1 meaning `no alignment requirements`,
+// this caused hard to spot bugs:
+//
----------------
jfb wrote:
> I don't think the prior history is useful here.
Indeed. I removed it.
================
Comment at: llvm/include/llvm/Support/Alignment.h:99
+// undefined (0) alignment.
+struct MaybeAlign final {
+private:
----------------
jfb wrote:
> Why is this better than `using MaybeAligned = Optional<Align>`?
> Do we care about packing that extra bit?
> It also seems that `Align` can have a shift of `255` and this can't.
When the data type for `Align` was `unsigned` or `uint64_t`, `llvm::Optional` was costly in terms of storage
```
sizeof(llvm::Optional<unsigned>) == 8
sizeof(llvm::Optional<uint64_t>) == 16
```
But with Align using `uint8_t` I agree it's not a big deal anymore `sizeof(llvm::Optional<uint8_t>) == 2`
I've added a comment about `Align::ShiftValue` being less than 64 by construction.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64790/new/
https://reviews.llvm.org/D64790
More information about the llvm-commits
mailing list