This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] Remove AllocaInst::setAlignment(unsigned)
ClosedPublic

Authored by gchatelet on Sep 27 2019, 8:00 AM.

Event Timeline

gchatelet created this revision.Sep 27 2019, 8:00 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 27 2019, 8:00 AM
courbet added inline comments.Sep 27 2019, 8:18 AM
llvm/lib/Target/AArch64/AArch64StackTagging.cpp
461

This could be an align, with a static_assert that kTagGranuleSize is > 0

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
2909

This is an Align because the 0 case has been checked just above.

gchatelet updated this revision to Diff 222372.Sep 30 2019, 1:48 AM
  • Address comments
gchatelet updated this revision to Diff 222400.Sep 30 2019, 5:53 AM
gchatelet marked an inline comment as done.
  • Address comments
gchatelet marked an inline comment as done.Sep 30 2019, 6:01 AM
courbet accepted this revision.Sep 30 2019, 6:03 AM
This revision is now accepted and ready to land.Sep 30 2019, 6:03 AM
This revision was automatically updated to reflect the committed changes.
arichardson added inline comments.
llvm/trunk/lib/Target/AArch64/AArch64StackTagging.cpp
65 ↗(On Diff #222409)

Can't the Align ctor be constexpr? Will this result in a Log2 call at runtime?

gchatelet marked an inline comment as done.Sep 30 2019, 7:18 AM
gchatelet added inline comments.
llvm/trunk/lib/Target/AArch64/AArch64StackTagging.cpp
65 ↗(On Diff #222409)

The implementation of Log2 is quite complex and depends on the implementation of countLeadingZeros.
GCC, clang, ICC can compute the value at compile time but there may be some compiler that can't.
https://godbolt.org/z/ytwWHn

I could solve this issue by introducing a LogAlign() function returning an Align. This function could be constexpr. What do you think? Is it worth the additional complexity?

arichardson added inline comments.Sep 30 2019, 11:17 AM
llvm/trunk/lib/Target/AArch64/AArch64StackTagging.cpp
65 ↗(On Diff #222409)

Since there won't a global ctor at -O2 or higher it probably doesn't matter.

I'm not sure if there have been any of these cases yet, but I would think that having a LogAlign/Align::fromLog()/etc. might still be useful for some cases where you get the alignment as log2 instead of in bytes?
This would avoid additional work for the compiler and might be easier to read since you can omit the 1<<N.

gchatelet marked an inline comment as done.Oct 2 2019, 4:29 AM
gchatelet added inline comments.
llvm/trunk/lib/Target/AArch64/AArch64StackTagging.cpp
65 ↗(On Diff #222409)

I'll wait a bit for the LogAlign to check if it's useful.
Meanwhile you may want to have a look at D68329 where I allow constexpr Align.