This is an archive of the discontinued LLVM Phabricator instance.

[WebAssmebly] Add support for defined wasm globals in MC and lld
ClosedPublic

Authored by sbc100 on Apr 29 2020, 3:41 PM.

Details

Summary

This change add supprot for defined wasm globas in the .s format
and in the MC layer.

Currently there is no support custom initialization and all wasm
globals are initialized to zero.

Fixes: PR45742

Diff Detail

Event Timeline

sbc100 created this revision.Apr 29 2020, 3:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 3:41 PM
sbc100 retitled this revision from [WebAssmebly] Add support for defined wasm in MC and lld to [WebAssmebly] Add support for defined wasm globals in MC and lld.Apr 29 2020, 3:42 PM
sbc100 added a reviewer: aardappel.
aardappel added inline comments.Apr 30 2020, 8:56 AM
lld/test/wasm/globals.s
5

I presume there's a reason or precedent why this is not .global ?

13

You were saying earlier there's a lot of boilerplate to writing a function in asm.. this seems pretty reasonable?

31

Are these labels required and if so, why?

llvm/lib/MC/WasmObjectWriter.cpp
797

So why is this section entirely new? Where did __stackpointer etc go previously?

llvm/test/MC/WebAssembly/globals.s
4

I guess having almost the same test in LLD and MC is unavoidable?

sbc100 marked 5 inline comments as done.Apr 30 2020, 11:24 AM
sbc100 added inline comments.
lld/test/wasm/globals.s
5

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.

13

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.

31

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.

llvm/lib/MC/WasmObjectWriter.cpp
797

Because __stack_pointer is always undefined right up until the linker synthesizes it.

llvm/test/MC/WebAssembly/globals.s
4

Yes, I think inputs will look similar for simple test cases.

aardappel accepted this revision.Apr 30 2020, 12:00 PM
aardappel added inline comments.
lld/test/wasm/globals.s
5

might be nice to standardize on .global then? up to you.

13

end_function is an actual instruction, not just a marker, so that's probably not a good idea.

This revision is now accepted and ready to land.Apr 30 2020, 12:00 PM
sbc100 marked 2 inline comments as done.Apr 30 2020, 12:36 PM
sbc100 added inline comments.
lld/test/wasm/globals.s
5

You mean .globl ? . it seem that that would be the logical choice since that is what MC always outputs.

13

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.

This revision was automatically updated to reflect the committed changes.
aardappel added inline comments.Apr 30 2020, 1:35 PM
lld/test/wasm/globals.s
13

We define end_block, end_loop etc. as distinct instructions in tablegen defs and thuout LLVM, they just map to the same opcode. The asm output is just a reflection of that.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/40961/steps/check-llvm%20msan/logs/stdio

FAIL: LLVM :: ObjectYAML/wasm/global_section.yaml (28409 of 37227)
******************** TEST 'LLVM :: ObjectYAML/wasm/global_section.yaml' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/yaml2obj /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/test/ObjectYAML/wasm/global_section.yaml | /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/obj2yaml | /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/FileCheck /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/test/ObjectYAML/wasm/global_section.yaml
--
Exit Code: 2

Command Output (stderr):
--
YAML:13:28: error: invalid number
          Value:           -5
                           ^~
yaml2obj: error: failed to parse YAML input: Invalid argument
Error reading file: <stdin>: The file was not recognized as a valid object file
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/FileCheck /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/test/ObjectYAML/wasm/global_section.yaml

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failing Tests (1):
  LLVM :: ObjectYAML/wasm/global_section.yaml


Testing Time: 192.29s
This comment was removed by vitalybuka.

Sorry for the breakage.. I have a simple fix in flight.