[PATCH] D139181: [lld][Alignment][NFC] Use Align instead of log2 of alignment in Wasm Sections

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added a comment.

In D139181#4003549 <https://reviews.llvm.org/D139181#4003549>, @MaskRay wrote:

> In D139181#3970123 <https://reviews.llvm.org/D139181#3970123>, @gchatelet wrote:
>
>> In D139181#3967018 <https://reviews.llvm.org/D139181#3967018>, @MaskRay wrote:
>>
>>> Drive-by: `llvm::Align` probably won't improve lld/ELF (and the extra dependency `llvm/Support/Alignment.h` and MathExtras.h is somewhat unnecessary). I've checked using p2align for `lld::elf::Symbol`, but I think it won't decrease the struct size and will likely lead to no less code size in most call sites.
>>
>> Thx @MaskRay. I think I should have started by explaining the rationale here. My apologies for that.
>>
>> The main reason for this change is not code size nor performance (although it can help a bit) but making sure the semantic is correct.
>> In `lld` alignment is specified as `uint8_t`, `uint32_t`, or `uint64_t`. Sometimes it refers to the p2align semantic, sometimes it refers to powers of two alignments with no clear explanation of what the `0` value is for (could be : "consider 0 as a 1",  "0 is forbidden", "0 => no alignment requirements, 1 => align to 1", "0 is a sentinel for the value not being set yet").
>> It also removes the need to check invariants, since they hold by definition.
>>
>>    % git grep isPowerOf2_  
>>   COFF/Chunks.cpp:  assert(isPowerOf2_32(c->getAlignment()));
>>   COFF/Chunks.h:    assert(llvm::isPowerOf2_32(align) && "alignment is not a power of 2");
>>
>> Introducing this type makes sure we can consistently reason about the value. I understand that this comes at the cost of additional dependencies and probably increased compile time, but it already caught bugs (from the RFC <https://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html>, this one a few days ago <https://reviews.llvm.org/D138784>) so I think it has value.
>> Even if there are no bugs today, the type helps when refactoring as the semantic is well defined.
>>
>> Let me know what you think before I continue working on this. Meanwhile I'm putting D138778 <https://reviews.llvm.org/D138778>, D139205 <https://reviews.llvm.org/D139205>, D139206 <https://reviews.llvm.org/D139206> on hold.
>
> llvm/ uses of alignment are very different from lld/ uses of alignment. lld/{COFF,ELF,wasm,MachO} are all different.
> They are just different implementations put into one directory. One port preferring alignment and another preferring p2align doesn't imply inconsistency in the code base.
>
> lld/COFF uses p2Align, so using `llvm::Align` may make sense.
> For ELF, I think using `llvm::Align` will just lead to more log2 or pow2 operations and more conversions into integral types, and makes us lose the ability to preserve 0 in case of relocatable links.

I've reverted the patches since they don't seem to provide much benefit. Would you also want me to revert this one?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139181/new/

https://reviews.llvm.org/D139181