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

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet added inline comments.


================
Comment at: lld/wasm/InputChunks.h:112
              uint32_t flags = 0)
-      : name(name), file(f), alignment(alignment), flags(flags), sectionKind(k),
-        live(!config->gcSections), discarded(false) {}
+      : name(name), file(f), alignment(1ULL << alignment), flags(flags),
+        sectionKind(k), live(!config->gcSections), discarded(false) {}
----------------
sbc100 wrote:
> It would be nice if there was a way to explicitly construct an llvm::Align from a p2align constant such as this?  fromP2Align?
We could, by making `LogValue` public [here](https://github.com/llvm/llvm-project/blob/495acf98da4b37bbcf58d1f14175a01d85d2b8f5/llvm/include/llvm/Support/Alignment.h#L55-L64).
I was reluctant to do so because of the lack of use cases but it seems to be a thing in the linker.


================
Comment at: lld/wasm/OutputSegment.cpp:28
+                    << " size=" << inSeg->getSize() << " align="
+                    << Log2(inSeg->alignment) << " at:" << size << "\n");
   inSeg->outputSeg = this;
----------------
sbc100 wrote:
> How about we skip the Log2 here and write `p2align=` in the log message just to be explict?
wait, I thought `p2align` was the log2 of alignment?
So either `"align=" << align` or `"p2_align=" << Log2(align)`.

Let me know if I misunderstood.


================
Comment at: lld/wasm/Writer.cpp:291
   for (OutputSegment *seg : segments) {
-    out.dylinkSec->memAlign = std::max(out.dylinkSec->memAlign, seg->alignment);
-    memoryPtr = alignTo(memoryPtr, 1ULL << seg->alignment);
+    out.dylinkSec->memAlign =
+        std::max(out.dylinkSec->memAlign, Log2(seg->alignment));
----------------
sbc100 wrote:
> Can we use Align for the `memAlign` member here too?
Yes, I intend to fix it all but it needs to be incremental otherwise it's hard to review. Same for the Ditto above, it will be converted once `SyntheticMergedChunk` takes an `Align`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139181