[PATCH] D61698: [COFF] Store alignment in log2 form, NFC

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/Chunks.cpp:860
+MergeChunk::MergeChunk(uint8_t P2Align)
+    : Builder(StringTableBuilder::RAW, 1U << P2Align) {
+  this->P2Align = P2Align;
----------------
I'd think we should always pass a power-of-two instead of log2 to constructors for the sake of consistency.


================
Comment at: lld/COFF/Chunks.h:47-48
 
+// The log base 2 of the largest section alignment, which is log2(8192), or 13.
+enum : unsigned { Log2MaxSectionAlignment = 13 };
+
----------------
Is this enforced by the file format?


================
Comment at: lld/COFF/Chunks.h:73
+    Align = Align ? Align : 1;
+    assert(llvm::isPowerOf2_32(Align) && "alignment is not a power of 2");
+    P2Align = llvm::Log2_32(Align);
----------------
If this can be triggered by feeding a bad file, this should be a call of error() instead of assert().


================
Comment at: lld/COFF/Chunks.h:136
+  // The alignment of this chunk. The writer uses the value. Maximum object file
+  // alignment is 2**13, or 8192, so valid values are 0 to 13.
+  uint8_t P2Align = 0;
----------------
Instead of emphasizing that the maximum alignment is 2^13, can we support an alignment up to a reasonable upper limit, say, 2^32? That's apparently large enough, and by doing that, we do not have to explain a trivia that what value is the maximum alignment value we can express by COFF. (And the runtime cost of doing that is virtually nothing.)


================
Comment at: lld/COFF/Driver.cpp:1744-1745
 
     CommonChunk *C = DC->getChunk();
-    C->Alignment = std::max(C->Alignment, Alignment);
+    C->setLog2Alignment(std::max(C->getLog2Alignment(), P2Align));
   }
----------------
Can you eliminate {set,get}Log2Alignment and consistently use {set,get}Alignment, so that the details about how it is represented internally is hidden from the user?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61698