This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][InstrEmitter] Foundation for multivalue call lowering
ClosedPublic

Authored by tlively on Dec 13 2019, 11:02 AM.

Details

Summary

WebAssembly is unique among upstream targets in that it does not at
any point use physical registers to store values. Instead, it uses
virtual registers to model positions in its value stack. This means
that some target-independent lowering activities that would use
physical registers need to use virtual registers instead for
WebAssembly and similar downstream targets. This CL generalizes the
existing usesPhysRegsForPEI lowering hook to
usesPhysRegsForValues in preparation for using it in more places.

One such place is in InstrEmitter for instructions that have variadic
defs. On register machines, it only makes sense for these defs to be
physical registers, but for WebAssembly they must be virtual registers
like any other values. This CL changes InstrEmitter to check the new
target lowering hook to determine whether variadic defs should be
physical or virtual registers.

These changes are necessary to support a generalized CALL instruction
for WebAssembly that is capable of returning an arbitrary number of
arguments. Fully implementing that instruction will require additional
changes that are described in comments here but left for a follow up
commit.

Diff Detail

Unit TestsFailed

Event Timeline

tlively created this revision.Dec 13 2019, 11:02 AM

Unit tests: pass. 60863 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

In the description you say " .. unique among upstream targets", and later "... WebAssembly and similar downstream targets.". Was it deliberate to use both "upstream" and "downstream", what do these term mean here? Also the "unique" statement seems to contract the "and similar" statement.

llvm/test/CodeGen/WebAssembly/multivalue.ll
2

Is this test also now testing tail call somehow?

tlively marked an inline comment as done.Dec 13 2019, 3:41 PM

In the description you say " .. unique among upstream targets", and later "... WebAssembly and similar downstream targets.". Was it deliberate to use both "upstream" and "downstream", what do these term mean here? Also the "unique" statement seems to contract the "and similar" statement.

Yes, this was deliberate. Upstream targets are targets in the LLVM source tree, and downstream targets are targets that people add to LLVM without checking them into the main repo. An example of a downstream target that is similar to WebAssembly is the TVM architecture for the TON blockchain network.

llvm/test/CodeGen/WebAssembly/multivalue.ll
2

Yes, one of the new tests below has a musttail call, which errors out if tail-call is not enabled.

Mostly LGTM to me

llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
837

Can't variadic-def call instructions also have implicit defs, such as $argument, $sp32, and $sp64?

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
512–513

I think the intention of D64643 was this file only contains methods that can work on unsigned Opc so that they can work equally on MachineInstrs and MCInsts. I now see that this function is not used for MCInsts now, but I think it'd better to make functions in this file consistently take unsigned Opc?

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
219

Why take out the wrapper?

llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
63

Just to clarify, even if variable_ops is within (ins), if variadicOpsAreDefs is 1, are they considered as outputs? What happens if we put variable_ops in (outs)?

tlively marked 4 inline comments as done.Dec 16 2019, 1:04 PM
tlively added a subscriber: dmitry.
tlively added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
837

I suppose they probably could, but I think it would complicate the logic to support mixed virtual and physical variadic and implicit arguments. I'm not sure it's worth trying to do when we don't have any instructions that would test that code path. Maybe @dmitry has a need for such an instruction?

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
512–513

We need the MachineInstr in the new case below because the callee op number depends on the number of defs that come before it. Are you suggesting that we move this function to a different file?

llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
219

The wrapper is there to enable pattern matching and is removed by the normal tablegen patterns as well.

llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td
63

That's correct. Tablegen errors out if you try to put the variable_ops in the (outs). This seems like something that could be cleaned up by someone with sufficient motivation, but that person is not me right now!

tlively updated this revision to Diff 234133.Dec 16 2019, 1:30 PM
  • Check isVariadic explicitly in InstrEmitter

Unit tests: pass. 60863 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

tlively updated this revision to Diff 234161.Dec 16 2019, 2:46 PM
  • Add OpInfo check to avoid OOB when there are only variadic ops

Unit tests: pass. 60863 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: diff.json, CMakeCache.txt, console-log.txt, test-results.xml

aheejin added inline comments.Dec 16 2019, 3:59 PM
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
837

Oh what I meant was, every wasm instruction implicitly defines ARGUMENT register, which is treated as a physical register. Turns out SP32 and SP64 are implicitly defined by only these two instructions, so I think this should be irrelevant.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
512–513

Either that, or leave this function here and writing a couple more line in the user side..?

tlively marked 2 inline comments as done.Jan 17 2020, 2:56 PM
tlively added inline comments.
llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
837

So yes, nodes can implicitly define physical registers, but that is not sufficient to make NumResults > NumDefs, since NumResults is the number of explicit outputs of the node. HasPhysRegOuts is only true when there are explicit ouputs of the node that need to be placed in physical registers. That's why it is correct to set it to false when HasVRegVariadicDefs is true. The implicit physical outputs are not affected by any of this.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
512–513

I would be ok moving this function to a different file, but I'm not sure which file that would be. This file contains all the helper functions for WebAssembly ops as far as I can tell, so I would also be ok leaving it here. It makes sense to generalize over MachineInstr and MCInst where possible, but that's just not possible in this case.

tlively updated this revision to Diff 238900.Jan 17 2020, 2:56 PM
  • Rebase on master

@aheejin, there are no changes I am currently planning to make to this, but I am happy to continue the inline discussions.

Unit tests: fail. 61975 tests passed, 1 failed and 783 were skipped.

failed: LLVM.Bindings/Go/go.test

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

aheejin accepted this revision.Jan 21 2020, 9:39 AM

Sorry for the delayed reply. I somehow assumed this CL was still in progress and didn't know it was finalized long ago! LGTM.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
512–513

I'd be also OK with leaving this function in this file in the current state, but isn't this function gonna be reduced to ~5 lines after we delete all other CALL_VOID and CALL_type variants? This function may not be really about opcodes by that point then. How about moving this to WebAssemblyUtilities.cpp/h or something?

This revision is now accepted and ready to land.Jan 21 2020, 9:39 AM

Sorry for the delayed reply. I somehow assumed this CL was still in progress and didn't know it was finalized long ago! LGTM.

No problem at all! I wasn't sure whether I would want to change it until I commented.

tlively marked an inline comment as done.Jan 21 2020, 10:59 AM
tlively added inline comments.
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
512–513

Yes, it will become much shorter. WebAssemblyUtilities sounds good.

This revision was automatically updated to reflect the committed changes.