[PATCH] D92952: [WebAssembly] Support COMDAT sections in assembly syntax

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Great!



================
Comment at: llvm/lib/MC/MCParser/WasmAsmParser.cpp:172
+    if (Group) {
+      if (parseGroup(GroupName))
+        return true;
----------------
Combine into single condition?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1370
 
+    LLVM_DEBUG(dbgs() << "Processing Section " << SectionName << "  group "
+                      << Section.getGroup() << "\n";);
----------------
Did you mean to leave this in?


================
Comment at: llvm/test/MC/WebAssembly/comdat-asm.ll:28
+; CHECK:        .section        .text.basicInlineFn,"G",@,basicInlineFn,comdat
+; CHECK-NEXT:        .weak   basicInlineFn                   # -- Begin function basicInlineFn
+; CHECK-NEXT:        .type   basicInlineFn, at function
----------------
I think we normally run with `-asm-verbose=false` to avoid these extra comments.


================
Comment at: llvm/test/MC/WebAssembly/comdat-sections.s:5
+        .globl foo
+        .type foo, at function
+foo:
----------------
I normally drop the `.type foo, at function` this its not needed.


================
Comment at: llvm/test/MC/WebAssembly/comdat-sections.s:23
+
+
+
----------------
Just one empty line here?


================
Comment at: llvm/test/MC/WebAssembly/comdat.ll:5
+; These RUN lines verify the ll direct-to-object path, the ll->asm path, and the
+; object output via asm.
 
----------------
Do we still need `comdat-asm.ll` given that this test runs in both modes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92952