Page MenuHomePhabricator

[WebAssembly] Allow multivalue types in block signature operands
ClosedPublic

Authored by tlively on Oct 11 2019, 2:06 PM.

Details

Summary

Renames ExprType to the more apt BlockType and adds a variant for
multivalue blocks. Currently non-void blocks are only generated at the
end of functions where the block return type needs to agree with the
function return type, and that remains true for multivalue
blocks. That invariant means that the actual signature does not need
to be stored in the block signature MachineOperand because it can be
inferred by WebAssemblyMCInstLower from the return type of the
parent function. WebAssemblyMCInstLower continues to lower block
signature operands to immediates when possible but lowers multivalue
signatures to function type symbols. The AsmParser and Disassembler
are updated to handle multivalue block types as well.

Diff Detail

Event Timeline

tlively created this revision.Oct 11 2019, 2:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 2:06 PM
sbc100 added inline comments.Oct 11 2019, 2:42 PM
llvm/tools/llvm-mc/llvm-mc.cpp
520

This change to remove the context creation seems seperate. If so can you split it out? That was this change can stay wasm specific.

tlively marked an inline comment as done.Oct 11 2019, 3:24 PM
tlively added inline comments.
llvm/tools/llvm-mc/llvm-mc.cpp
520

It's not separate, unfortunately. In order to create a wasm symbol from the disassembler we need a bunch of target information that this target contains that the old one did not have.

tlively marked an inline comment as done.Oct 11 2019, 3:25 PM
tlively added inline comments.
llvm/tools/llvm-mc/llvm-mc.cpp
520

I am happy to reconsider the wisdom of creating a symbol in the disassembler, though.

sbc100 added inline comments.Oct 11 2019, 3:38 PM
llvm/tools/llvm-mc/llvm-mc.cpp
520

I was confused because the existing disassembly already needs a context and even creates one saying: // Set up the MCContext for creating symbols and MCExpr's.. So that approach seems justified and consistent with the existing code. I'm not clear why it wasn't done this way to begin with.

tlively marked an inline comment as done.Oct 11 2019, 3:49 PM
tlively added inline comments.
llvm/tools/llvm-mc/llvm-mc.cpp
520

Yeah it was surprising that the previous code didn't work. With the previous code, asking the context for a symbol returned a generic MCSymbol. With the new code it returns a MCSymbolWasm, which we need to be able to set the symbol type to wasm::WASM_SYMBOL_TYPE_FUNCTION.

dschuff added inline comments.Oct 11 2019, 4:51 PM
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
135

The invariant that only fallthrough-return blocks are allowed to be multivalue should probably be restated here.
edit: I guess it's not just fallthrough-return blocks, right? Just the last block, even if it has an explicit return?
We should probably have some more tests for those cases.

tlively marked an inline comment as done.Oct 11 2019, 5:21 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
135

Right, it's the last block and the last block within that block recursively. In fact this only happens with explicit returns inside the blocks (or blocks that are otherwise never exited) because blocks that could be fallthrough return blocks instead set their results to a local then have a local.get in the return position.

I will expand this comment to reiterate this invariant.

I only added one additional test for this case because the algorithm for determining when to set block types was not changed and is already tested in cfg-stackify.ll.

tlively updated this revision to Diff 224715.Oct 11 2019, 5:29 PM
  • Explain more about multivalue types in WebAssembly::BlockType

Nice! Mostly LGTM.

Currently non-void blocks are only generated at the end of functions where the block return type needs to agree with the function return type, and that remains true for multivalue blocks. That invariant means that the actual signature does not need to be stored in the block signature MachineOperand because it can be inferred by WebAssemblyMCInstLower from the return type of the parent function.

I guess this is a tentative state before you implement the rest of the proposal in full, right? If other blocks are able to return multivalue, are we gonna change their operands to also take typeindex?

llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
224

What are septet values, and when is Val negative? All values of BlockType look unsigned. Maybe I'm missing something..?

227

When does this happen?

239

Disassembler is not going to print multivalue signatures then? Is this tentative or permanent?

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

Nit: Do we need llvm:: here?

aheejin added inline comments.Oct 11 2019, 5:55 PM
llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
1248

It's preexisting, but I think we should add END_TRY here too. Not sure if I can generate a test case that ends with end_try returning something easily though..

tlively marked 4 inline comments as done.Oct 11 2019, 6:02 PM
tlively added inline comments.
llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
224

See https://webassembly.github.io/multi-value/core/binary/instructions.html#control-instructions for details on the encoding. By septet value I just mean a group of 7 bits. That's where the & 0x7f comes from.

227

See wasm-error.txt for an example. Basically anytime you have a negative SLEB128 value here that occupies more than one byte, which is invalid according to the spec.

239

I don't think there's any way to access the type section from the disassembler, so this is permanent unless there's a large overhaul. cc @aardappel for the details.

llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
1248

Sounds good.

Nice! Mostly LGTM.

Currently non-void blocks are only generated at the end of functions where the block return type needs to agree with the function return type, and that remains true for multivalue blocks. That invariant means that the actual signature does not need to be stored in the block signature MachineOperand because it can be inferred by WebAssemblyMCInstLower from the return type of the parent function.

I guess this is a tentative state before you implement the rest of the proposal in full, right? If other blocks are able to return multivalue, are we gonna change their operands to also take typeindex?

Ping

Nice! Mostly LGTM.

Currently non-void blocks are only generated at the end of functions where the block return type needs to agree with the function return type, and that remains true for multivalue blocks. That invariant means that the actual signature does not need to be stored in the block signature MachineOperand because it can be inferred by WebAssemblyMCInstLower from the return type of the parent function.

I guess this is a tentative state before you implement the rest of the proposal in full, right? If other blocks are able to return multivalue, are we gonna change their operands to also take typeindex?

I don't think we have to change this, or if we do it could be considered a separate project. Up to now we have been ok not using the ability for blocks to produce values, so I don't see a pressing need to change that. When we do work on multivalue blocks, it will be more important to have them in Binaryen first.

tlively updated this revision to Diff 225080.Oct 15 2019, 10:56 AM
tlively marked 3 inline comments as done.
  • Address comments
aheejin accepted this revision.Oct 15 2019, 11:18 AM
This revision is now accepted and ready to land.Oct 15 2019, 11:18 AM
This revision was automatically updated to reflect the committed changes.
aardappel added inline comments.Oct 17 2019, 10:55 AM
llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
233

Is this necessary? A disassembler is really just a function that turns a range of bytes into a string, it having a "side effect" of creating symbols seems odd to me.