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

Mon Dec 5 01:25:22 PST 2022

gchatelet added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139181