[PATCH] D53842: [WebAssembly] Parsing missing directives to produce valid .o

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel added inline comments.


================
Comment at: include/llvm/ADT/StringSwitch.h:214
+    return std::move(Result);
+  }
+
----------------
dschuff wrote:
> aardappel wrote:
> > Since this is a core LLVM class, should I get someone in particular to review? Or am I better off writing the calling code differently?
> For our use case, we have `T = wasm::WasmSymbolType` and `R` defaults to `T`. Could we set `R` to `Optional<wasm::WasmSymbolType>` and just use `Default` instead of `NoDefault`?
That doesn't work because the existing `Default` always returns a `T`, i.e. there is no way to indicate a null `Optional`.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:282
+                                            Triple::wasm64);
+          // TODO: do we ideally want more generic handling like this code
+          // below instead of checkKnownSymbol?
----------------
dschuff wrote:
> aardappel wrote:
> > I could remove the code below, left in for code review purposes.
> In general, I think making the assembler dumb is not a bad thing. Also you could argue that conventions for symbol names are compiler/ABI conventions and should not be automatically assumed by the assembler. So I guess if we thought of it that way it would mean that we'd have something like this generic code here, and the compiler would be required to output a directive declaring the type of `__stack_pointer` as a global, right? I think that is actually better than making the assembler smart about the compiler's ABI. Even the factoring of the Assembler/MC code reflects this idea; it's why there's no good place to put that code if it were shared.
Ok, so I could make the InstPrinter print out this info, but really the only information that should be in the .S output is wether it is global (which is already in there, and I can recover using the commented code below), and the type of the var, which I can also infer in the assembler. So really I don't need to share the function, the only reason to do was that a) it happens the same in both places, and b) that if someone decides to refactor this code they'll be forced to look at both.

I could make the assembler dumber if the InstPrinter also outputs the type. This could be in a directive like `.global __stack_pointer i32` but there is no obvious prelude/post output in InstPrinter (on the wasm side). Or we could pack it all in the symbol reference somehow, like `__stack_pointer at GLOBAL:I32` (would maybe require changes from the general parser) or `__stack_pointer at GLOBAL_I32` (polute with wasm specific symbol types).


Repository:
  rL LLVM

https://reviews.llvm.org/D53842