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

[gchatelet]

Summary

I intend to slowly upgrade all alignments to the Align type in lld as well.
Some places talk about alignment in Bytes while other specify them as Log2(Bytes).
Let's make sure all of this is coherent.

[gchatelet]

• rebase and reformat

[arichardson]

lld/wasm/InputChunks.h

88

Reordering the fields would reduce padding.

lld/wasm/OutputSegment.h

41

Same here

[sbc100]

lld/wasm/InputChunks.h

112

It would be nice if there was a way to explicitly construct an llvm::Align from a p2align constant such as this? fromP2Align?

lld/wasm/OutputSegment.cpp

28

How about we skip the Log2 here and write p2align= in the log message just to be explict?

61

ditto

lld/wasm/Writer.cpp

291

Can we use Align for the memAlign member here too?

[gchatelet]

lld/wasm/InputChunks.h

112

We could, by making LogValue public here.
I was reluctant to do so because of the lack of use cases but it seems to be a thing in the linker.

lld/wasm/OutputSegment.cpp

28

wait, I thought p2align was the log2 of alignment?
So either "align=" << align or "p2_align=" << Log2(align).

Let me know if I misunderstood.

lld/wasm/Writer.cpp

291

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.

[sbc100]

lld/wasm/OutputSegment.cpp

28

At yes sorry. I think it would be more clear if the log message said p2align=.. but since your change didn't effect that we don't need to make that change here. Sorry for the noise.

[MaskRay]

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.

[gchatelet]

In 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, this one a few days ago) 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, D139205, D139206 on hold.

[MaskRay]

In D139181#3970123, @gchatelet wrote:

In 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, this one a few days ago) 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, D139205, 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.

[gchatelet]

In D139181#4003549, @MaskRay wrote:

In D139181#3970123, @gchatelet wrote:

In 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, this one a few days ago) 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, D139205, 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?

[MaskRay]

Thanks! I think @sbc100 will decide. From my look of these call sites, the number of Log2 call sites makes me feel that llvm::Align is probably not a good fit for lld/wasm as well.

[sbc100]

In D139181#4005658, @MaskRay wrote:

Thanks! I think @sbc100 will decide. From my look of these call sites, the number of Log2 call sites makes me feel that llvm::Align is probably not a good fit for lld/wasm as well.

I tend to agree. I also tend to think we should either use this in all linkers on none.

[gchatelet]

In D139181#4005658, @MaskRay wrote:

Thanks! I think @sbc100 will decide. From my look of these call sites, the number of Log2 call sites makes me feel that llvm::Align is probably not a good fit for lld/wasm as well.

Sounds good, I'll revert then.
Note that Log2 is a noop though.

lld/wasm/InputChunks.h

public:
   void generateRelocationCode(raw_ostream &os) const;
 
   bool isTLS() const { return flags & llvm::wasm::WASM_SEG_FLAG_TLS; }
 
   ObjFile *file;
   OutputSection *outputSec = nullptr;
   uint32_t comdat = UINT32_MAX;
   uint32_t inputSectionOffset = 0;
-  uint32_t alignment;
+  llvm::Align alignment;
   uint32_t flags;
 
   // Only applies to data segments.
   uint32_t outputSegmentOffset = 0;
   const OutputSegment *outputSeg = nullptr;
 
   // After assignAddresses is called, this represents the offset from
   // the beginning of the output section this chunk was assigned to.
   int32_t outSecOff = 0;
 
   uint8_t sectionKind : 3;
 
   // Signals that the section is part of the output.  The garbage collector,
   // and COMDAT handling can set a sections' Live bit.
   // If GC is disabled, all sections start out as live by default.
   unsigned live : 1;
 
   // Signals the chunk was discarded by COMDAT handling.
   unsigned discarded : 1;
 
 protected:
   InputChunk(ObjFile *f, Kind k, StringRef name, uint32_t alignment = 0,
              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) {}
   ArrayRef<uint8_t> data() const { return rawData; }
   uint64_t getTombstone() const;
 
   ArrayRef<WasmRelocation> relocations;
   ArrayRef<uint8_t> rawData;
 };
 
 // Represents a WebAssembly data segment which can be included as part of

lld/wasm/OutputSegment.h

public:
   bool isTLS() const { return name == ".tdata"; }
 
   StringRef name;
   bool isBss = false;
   uint32_t index = 0;
   uint32_t linkingFlags = 0;
   uint32_t initFlags = 0;
   uint32_t sectionOffset = 0;
-  uint32_t alignment = 0;
+  llvm::Align alignment;
   uint64_t startVA = 0;
   std::vector<InputChunk *> inputSegments;
 
   // Sum of the size of the all the input segments
   uint32_t size = 0;
 
   // Segment header
   std::string header;
 };
 
 } // namespace wasm
 } // namespace lld
 
 #endif // LLD_WASM_OUTPUT_SEGMENT_H

lld/wasm/OutputSegment.cpp

 
 namespace lld {
 
 namespace wasm {
 
 void OutputSegment::addInputSegment(InputChunk *inSeg) {
   alignment = std::max(alignment, inSeg->alignment);
   inputSegments.push_back(inSeg);
-  size = llvm::alignTo(size, 1ULL << inSeg->alignment);
+  size = llvm::alignTo(size, inSeg->alignment);
   LLVM_DEBUG(dbgs() << "addInputSegment: " << inSeg->name << " oname=" << name
-                    << " size=" << inSeg->getSize()
-                    << " align=" << inSeg->alignment << " at:" << size << "\n");
+                    << " size=" << inSeg->getSize() << " align="
+                    << Log2(inSeg->alignment) << " at:" << size << "\n");
   inSeg->outputSeg = this;
   inSeg->outputSegmentOffset = size;
   size += inSeg->getSize();
 }
 
 // This function scans over the input segments.
 //
 // It removes MergeInputChunks from the input section array and adds
for (InputChunk *s : inputSegments) {
     // A segment should not make it here unless its alive
     assert(ms->live);
 
     auto i = llvm::find_if(mergedSegments, [=](SyntheticMergedChunk *seg) {
       return seg->flags == ms->flags && seg->alignment == ms->alignment;
     });
     if (i == mergedSegments.end()) {
       LLVM_DEBUG(llvm::dbgs() << "new merge segment: " << name
-                              << " alignment=" << ms->alignment << "\n");
-      auto *syn = make<SyntheticMergedChunk>(name, ms->alignment, ms->flags);
+                              << " alignment=" << Log2(ms->alignment) << "\n");
+      auto *syn =
+          make<SyntheticMergedChunk>(name, Log2(ms->alignment), ms->flags);
       syn->outputSeg = this;
       mergedSegments.push_back(syn);
       i = std::prev(mergedSegments.end());
       newSegments.push_back(syn);
     } else {
       LLVM_DEBUG(llvm::dbgs() << "adding to merge segment: " << name << "\n");
     }
     (*i)->addMergeChunk(ms);
   }
 
   for (auto *ms : mergedSegments)
     ms->finalizeContents();
 
   inputSegments = newSegments;
   size = 0;
   for (InputChunk *seg : inputSegments) {
-    size = llvm::alignTo(size, 1ULL << seg->alignment);
+    size = llvm::alignTo(size, seg->alignment);
     LLVM_DEBUG(llvm::dbgs() << "outputSegmentOffset set: " << seg->name
                             << " -> " << size << "\n");
     seg->outputSegmentOffset = size;
     size += seg->getSize();
   }
 }
 
 } // namespace wasm
 } // namespace lld

lld/wasm/SyntheticSections.cpp

if (!symtabEntries.empty()) {
     sub.writeTo(os);
   }
 
   if (dataSegments.size()) {
     SubSection sub(WASM_SEGMENT_INFO);
     writeUleb128(sub.os, dataSegments.size(), "num data segments");
     for (const OutputSegment *s : dataSegments) {
       writeStr(sub.os, s->name, "segment name");
-      writeUleb128(sub.os, s->alignment, "alignment");
+      writeUleb128(sub.os, Log2(s->alignment), "alignment");
       writeUleb128(sub.os, s->linkingFlags, "flags");
     }
     sub.writeTo(os);
   }
 
   if (!initFunctions.empty()) {
     SubSection sub(WASM_INIT_FUNCS);
     writeUleb128(sub.os, initFunctions.size(), "num init functions");

lld/wasm/Writer.cpp

void Writer::layoutMemory() {
 
   // Arbitrarily set __dso_handle handle to point to the start of the data
   // segments.
   if (WasmSym::dsoHandle)
     WasmSym::dsoHandle->setVA(dataStart);
 
   out.dylinkSec->memAlign = 0;
   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));
+    memoryPtr = alignTo(memoryPtr, seg->alignment);
     seg->startVA = memoryPtr;
     log(formatv("mem: {0,-15} offset={1,-8} size={2,-8} align={3}", seg->name,
-                memoryPtr, seg->size, seg->alignment));
+                memoryPtr, seg->size, Log2(seg->alignment)));
 
     if (!config->relocatable && seg->isTLS()) {
       if (WasmSym::tlsSize) {
         auto *tlsSize = cast<DefinedGlobal>(WasmSym::tlsSize);
         setGlobalPtr(tlsSize, seg->size);
       }
       if (WasmSym::tlsAlign) {
         auto *tlsAlign = cast<DefinedGlobal>(WasmSym::tlsAlign);
-        setGlobalPtr(tlsAlign, int64_t{1} << seg->alignment);
+        setGlobalPtr(tlsAlign, seg->alignment.value());
       }
       if (!config->sharedMemory && WasmSym::tlsBase) {
         auto *tlsBase = cast<DefinedGlobal>(WasmSym::tlsBase);
         setGlobalPtr(tlsBase, memoryPtr);
       }
     }
 
     memoryPtr += seg->size;