Page MenuHomePhabricator

DebugInfo: use uint32_t instead of uint64_t for alignment
ClosedPublic

Authored by vleschuk on Oct 14 2016, 7:35 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

vleschuk updated this revision to Diff 74683.Oct 14 2016, 7:35 AM
vleschuk retitled this revision from to DebugInfo: introduce DIAlignment type.
vleschuk updated this object.
vleschuk added a subscriber: llvm-commits.
aprantl edited edge metadata.Oct 14 2016, 9:26 AM

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?

probinson added inline comments.Oct 14 2016, 10:08 AM
include/llvm/IR/DebugInfoMetadata.h
518 ↗(On Diff #74683)

I think AlignInBits should come after OffsetInBits now, rather than between the two uint64_t fields?

vleschuk added inline comments.Oct 17 2016, 3:17 AM
include/llvm/IR/DebugInfoMetadata.h
518 ↗(On Diff #74683)

Agreed, will fix.

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?

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).

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.

vleschuk updated this revision to Diff 74898.Oct 17 2016, 1:58 PM
vleschuk retitled this revision from DebugInfo: introduce DIAlignment type to DebugInfo: use uint32_t instead of uint64_t for alignment.
vleschuk edited edge metadata.
  • Use uint32_t directly instead of new typedef
  • Place uint32_t alignment member after all uint64_t members in DIType class
vleschuk marked 2 inline comments as done.Oct 17 2016, 1:59 PM
aprantl accepted this revision.Oct 17 2016, 2:10 PM
aprantl edited edge metadata.

Small nitpicks inline, but otherwise this looks good to me!

lib/IR/LLVMContextImpl.h
352 ↗(On Diff #74898)

Here we could reorder the fields so AlignInBits isn't stuck between two 64-bit values.

433 ↗(On Diff #74898)

Same here.

This revision is now accepted and ready to land.Oct 17 2016, 2:10 PM

Small nitpicks inline, but otherwise this looks good to me!

Thanks a lot! Fixed these. Will commit as soon as https://reviews.llvm.org/D25621 is accepted.

This revision was automatically updated to reflect the committed changes.