Page MenuHomePhabricator

[CodeGen][WebAssembly] Better lowering for WASM_SYMBOL_TYPE_GLOBAL symbols
ClosedPublic

Authored by wingo on May 5 2021, 8:22 AM.

Details

Summary

As we have been missing support for WebAssembly globals on the IR level,
the lowering of WASM_SYMBOL_TYPE_GLOBAL to IR was incomplete. This
commit fleshes out the lowering support, lowering references to and
definitions of addrspace(1) values to correctly typed
WASM_SYMBOL_TYPE_GLOBAL symbols.

Depends on D101608.

Diff Detail

Event Timeline

wingo created this revision.May 5 2021, 8:22 AM
wingo requested review of this revision.May 5 2021, 8:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2021, 8:22 AM
wingo added a comment.May 5 2021, 8:29 AM

Hi! Please take a look. The code duplication is horrible and suggestions to fix are most welcome. Normally, overriding emitGlobalVariable would be sufficient, but we also need to visit uses of Wasm globals, even if they aren't defined in the compilation unit. The code in GetGlobalAddressSymbol would be sufficient, but we also need to visit definitions that aren't used. Irritating!

@sbc100, you know a lot more about this symbol stuff than I do. Could you take a look at this?

sbc100 accepted this revision.May 5 2021, 4:01 PM

This looks great!

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
179

Why this? As it happens I think that globals are *all* thread local by default (at least we assume they are in the current threading ABI in llvm and emscripten). We don't have a way to share global between threads to (like we do with memory).

204

There is no actual initializer here.. it will always be zero I guess? Do we need a TODO?

218

Why not leave this code alone and omit the emitGlobalType above?

307

Similar to other PR.. I wish we could find a better work than Object in isObjectAddressSpace

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
68

Regarding the duplication.. I think we already suffer from a little of that with the existing code for handling functions (See WasmSym->setSignature below and in emitFunctionBodyStart and in emitEndOfAsmFile). So I guess the makes it ok ? :-/

This revision is now accepted and ready to land.May 5 2021, 4:01 PM
wingo updated this revision to Diff 343411.May 6 2021, 8:00 AM
wingo marked 2 inline comments as done.

Rename "object" to "managed", and address feedback

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
179

Good point; I hadn't thought about this. For context I was looking at what the base "getGlobalVariable" does and where it has conditional blocks for non-default flags, and asserting that no one had made non-default settings on the symbol.

But... semantically what's the story here? I guess that although you can share globals between instances, you can't post them to a web worker, so they are always thread-local.

Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?

204

Added the TODO. This one is tricky because there's no MC support for global initializers yet, AFAIU.

Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by end, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the .globaltype line or as a new .globalvalue SYM, EXPR form. WDYT? Would be a followup I guess.

218

Because we need to record the type of the extern global somewhere in the .s file. Will add to comment. Sound OK to you?

307

Changed to "managed"; lmk what you think!

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
68

Fair enough :)

sbc100 added inline comments.May 6 2021, 8:19 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
179

Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?

I guess so .. at least if we are talking about the current threading support on the web. In the future globals will most likely be share-able.. or on other targets they might even be shared by default. But given that llvm's thread support is currently only used by the web targets it would make sense to me yes. Does't have to be part of this change though.

204

Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by end, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the .globaltype line or as a new .globalvalue SYM, EXPR form. WDYT? Would be a followup I guess.

Yeah, I'm not sure about this either. Using actual instructions seems overkill, but at the same time matches the spec. I think it would be great if we could make it work... but also a more declarative mode might sense given the severe restrictions on instructions in cont expressions today.

How close are you to needing initializer values here? Can we punt for a little longer? :)

wingo added inline comments.May 10 2021, 5:08 AM
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
204

Thanks for review & sorry I forgot to reply :) Yeah can wait on the initializers -- in most cases I am interested in, the default value will be fine.

wingo updated this revision to Diff 344334.May 11 2021, 2:37 AM

Rename "managed" address space to "wasm var".

This revision was landed with ongoing or failed builds.May 11 2021, 2:48 AM
This revision was automatically updated to reflect the committed changes.