[PATCH] D26722: [WebAssembly] Add skeleton MC support for the Wasm container format

[Dan Gohman via Phabricator via llvm-commits]

sunfish marked 5 inline comments as done.
sunfish added inline comments.


================
Comment at: lib/MC/MCWasmStreamer.cpp:85
+  // them. This makes writing matching .o files easier.
+  if (Attribute == MCSA_IndirectSymbol) {
+    // Note that we intentionally cannot use the symbol data here; this is
----------------
t.p.northover wrote:
> I have a strong suspicion that only MachO has indirect symbols. Certainly when I put an llvm_unreachable in the ELF equivalent of this block nothing broke.
Sounds good. I removed the indirect symbols code and added a similar assert.


================
Comment at: lib/MC/WasmObjectWriter.cpp:79
+  typedef std::map<const MCSectionWasm *, std::pair<uint64_t, uint64_t>>
+      SectionSizesTy;
+
----------------
rnk wrote:
> seems unused
removed


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp:101-102
+                                            MCObjectWriter *OW) const {
+  if (Count == 0)
+    return true;
+
----------------
t.p.northover wrote:
> Is this really necessary? It's basically exactly what the for loop is doing below.
removed


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyAsmBackend.cpp:116
+
+  unsigned NumBytes = (Info.TargetSize + 7) / 8;
+  if (Value == 0)
----------------
t.p.northover wrote:
> llvm::alignTo does this.
Updated to use alignTo.


Repository:
  rL LLVM

https://reviews.llvm.org/D26722