This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] migrate DataLayout internal struct to llvm::Align
ClosedPublic

Authored by gchatelet on Sep 10 2019, 7:46 AM.

Details

Summary

This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

With this patch the sizeof(PointerAlignElem) struct goes from 20B to 16B, while sizeof(LayoutAlignElem) stays at 8B (the struct is padded)

Event Timeline

gchatelet created this revision.Sep 10 2019, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 7:46 AM
gchatelet edited the summary of this revision. (Show Details)Sep 10 2019, 7:52 AM
courbet added inline comments.Sep 10 2019, 8:00 AM
llvm/lib/IR/DataLayout.cpp
54

const

369

I would move this back to DataLayout::setAlignment, or ask for a review to the original authors. They might have had other reasons beyond storage for guarding against this size.

gchatelet marked 2 inline comments as done.Sep 10 2019, 8:07 AM
gchatelet added inline comments.
llvm/lib/IR/DataLayout.cpp
369

@majnemer it looks like the checks are only because of implementation details (from 1b9fc3a186aed6ec120688e4d957466f72db3ed5)
Would it make sense to remove them as well as the tests?

gchatelet updated this revision to Diff 219547.Sep 10 2019, 8:07 AM
  • Address comments
gchatelet marked an inline comment as not done.Sep 10 2019, 8:08 AM
gchatelet updated this revision to Diff 221012.Sep 20 2019, 6:15 AM
  • Adding asserts to setAlignment
courbet accepted this revision.Sep 20 2019, 6:29 AM
This revision is now accepted and ready to land.Sep 20 2019, 6:29 AM
gchatelet marked an inline comment as done.Sep 20 2019, 6:37 AM
gchatelet added inline comments.
llvm/lib/IR/DataLayout.cpp
369

@majnemer I'll submit the code as-is, if you have any concerns please let me know and I can fix it in subsequent patches. Thx!

This revision was automatically updated to reflect the committed changes.