Introduce DIAlignment typedef, actually change alignment type from uint64_t to uint32_t to save space. In dependent patches we shall have alignment field added to DIVariable family and switching from uint64_t to uint32_t will save 4 bytes per variable.
Details
Diff Detail
Event Timeline
I support this.
One question: What happens when we read bitcode that contains an alignment field with a value larger > 2^32?
Does the bitcode reader throw an error? Do we even care to support a non-breaking upgrade path for this?
include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
515–516 | I think AlignInBits should come after OffsetInBits now, rather than between the two uint64_t fields? |
include/llvm/IR/DebugInfoMetadata.h | ||
---|---|---|
515–516 | Agreed, will fix. |
We already do check this in frontend (Sema):
// Alignment calculations can wrap around if it's greater than 2**28. unsigned MaxValidAlignment = Context.getTargetInfo().getTriple().isOSBinFormatCOFF() ? 8192 : 268435456;
I have added check in BitcodeReader just in case.
After letting this sit for a few days, I think that using an unsigned (there doesn't seem to be much precedent for uint32_t) is the way to go. It's more readable than an opaque DIAlignment type (whose size isn't obvious from the name).
What about platforms where sizeof(unsigned) could be 2, standard does allow that, I'd rather use cstdint types.
- Use uint32_t directly instead of new typedef
- Place uint32_t alignment member after all uint64_t members in DIType class
Thanks a lot! Fixed these. Will commit as soon as https://reviews.llvm.org/D25621 is accepted.
I think AlignInBits should come after OffsetInBits now, rather than between the two uint64_t fields?