[PATCH] D79137: [WebAssmebly] Add support for defined wasm globals in MC and lld

[Sam Clegg via Phabricator via llvm-commits]

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


================
Comment at: lld/test/wasm/globals.s:5
+
+.globl _start
+.globl read_global
----------------
aardappel wrote:
> I presume there's a reason or precedent why this is not `.global` ?
I don't really know.  It seems that both ".global" and ".globl" are accepted in the input but when `.s` format is written it always seems to write the shorter form, so I assumed it was preferred.

Indeed, looking at the code it looks like GlobalDirective in  MCAsmInfo is always set to: `"\t.globl\t"`, even though to parser accepts the long form.


================
Comment at: lld/test/wasm/globals.s:13
+read_global:
+  .functype read_global () -> (i32)
+  global.get foo_global
----------------
aardappel wrote:
> You were saying earlier there's a lot of boilerplate to writing a function in asm.. this seems pretty reasonable?
You are right, its not too bad.    I wonder if we could make `end_function` optional though?   Also, IIRC that location of the `.functype` directive is tied (needlessly?) to this position at the start I think. 


================
Comment at: lld/test/wasm/globals.s:31
+
+foo_global:
+bar_global:
----------------
aardappel wrote:
> Are these labels required and if so, why?
Yes, these actually define the symbols.   

If you just say `.globaltype bar_global, f32` you are saying there is some global with that name and type, but you are not defining it.  Same with functions.   Creating the label like this actually creates the thing.

In the next phase we should probably require some kind of initialization too.  But that is not needed for this change.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:797
+
+  SectionBookkeeping Section;
+  startSection(Section, wasm::WASM_SEC_GLOBAL);
----------------
aardappel wrote:
> So why is this section entirely new? Where did `__stackpointer` etc go previously?
Because `__stack_pointer` is always undefined right up until the linker synthesizes it. 




================
Comment at: llvm/test/MC/WebAssembly/globals.s:4
+
+# Tests creating an accessing actual wasm globals
+
----------------
aardappel wrote:
> I guess having almost the same test in LLD and MC is unavoidable?
Yes, I think inputs will look similar for simple test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79137