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

[Sam Clegg via Phabricator via llvm-commits]

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


================
Comment at: lld/test/wasm/globals.s:5
+
+.globl _start
+.globl read_global
----------------
aardappel wrote:
> sbc100 wrote:
> > 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.
> might be nice to standardize on `.global` then? up to you.
You mean `.globl` ?  . it seem that that would be the logical choice since that is what MC always outputs.




================
Comment at: lld/test/wasm/globals.s:13
+read_global:
+  .functype read_global () -> (i32)
+  global.get foo_global
----------------
aardappel wrote:
> sbc100 wrote:
> > 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. 
> `end_function` is an actual instruction, not just a marker, so that's probably not a good idea.
Right.. but in that case maybe we should just call it `end` since that is the name of the instruction.

Also, this IIUC is not present at all in the way format.. so there is a precedent for not including this.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79137