[PATCH] D139181: [lld][Alignment][NFC] Use Align instead of log2 of alignment in Wasm Sections
Guillaume Chatelet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 2 09:02:13 PST 2022
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
More information about the llvm-commits
mailing list