[PATCH] D97657: [lld][WebAssembly] Initial support merging string data

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/wasm/InputChunks.h:216
+public:
+  SyntheticMergedDataSegment(StringRef name, uint32_t flags_,
+                             uint32_t alignment_)
----------------
dschuff wrote:
> Do we care about these lint checks? (I guess lld already has different style than LLVM?)
We do care yes, but I'm having hard time seeing what is wrong here since we use lowerCamal case for locals and params in lld.   i guess it must be the trailing `_` .. in which case maybe its false positive because it thinks we are using snake_case which would indeed be wrong.   I think we can ignore this one.


================
Comment at: lld/wasm/InputChunks.h:42
+    DataSegment,
+    Merge,
+    MergedSegment,
----------------
dschuff wrote:
> Is "Merge" a segment that can be merged, and "MergedSegment" after merging? Would "MergeableSegment" better reflect what it means?
Again taking my lead from ELF here which has `enum Kind { Regular, EHFrame, Merge, Synthetic, Output };`


================
Comment at: lld/wasm/InputFiles.cpp:435
+
+  // On a regular link we don't merge sections if -O0 (default is -O1). This
+  // sometimes makes the linker significantly faster, although the output will
----------------
dschuff wrote:
> Does clang explicitly pass `-O0` to the linker to opt out, or does the user have to do that with `-Wl`?
I don't think clang ever injects that linker flag, you would need to use `-Wl,`


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:798
+  DwarfStrSection = Ctx->getWasmSection(
+      ".debug_str", SectionKind::getMetadata(), wasm::WASM_SEG_FLAG_STRINGS);
   DwarfLocSection =
----------------
dschuff wrote:
> sbc100 wrote:
> > dschuff wrote:
> > > So this sets the section flag as a string section, but we just don't merge them yet?
> > Yes, currently this will not get used anywhere, or written the binary format.  If you prefer I can drop this part of the change and add it in the followup.
> I don't necessarily object; but you also mentioned above that the handling (or just the naming?) might be different for custom sections compared to custom sections.
Removed this... in fact I reverted this whole file!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97657