Page MenuHomePhabricator

[WebAssembly] Modified tablegen defs to have 2 parallel instuction sets.

Authored by aardappel on Jun 14 2018, 11:49 AM.



One for register based, much like the existing definitions,
and one for stack based (suffix _S).

This allows us to use registers in most of LLVM (which works better),
and stack based in MC (which results in a simpler and more readable
assembler / disassembler).

Tried to keep this change as small as possible while passing tests,
follow-up commit will:

  • Add reg->stack conversion in MI.
  • Fix asm/disasm in MC to be stack based.
  • Fix emitter to be stack based.

tests passing:
llvm-lit -v find test -name WebAssembly


Diff Detail


Event Timeline

aardappel created this revision.Jun 14 2018, 11:49 AM
dschuff added inline comments.Jun 14 2018, 1:04 PM
15 ↗(On Diff #151391)

There should probably be a bit more explanation here, saying that every instruction has the 2 representations, how they are used, how the oops/iops and asmstrings need to be different, the naming convention, etc.

467 ↗(On Diff #151391)

The real FIXME is to just remove CURRENT_MEMORY and GROW_MEMORY altogether (they are the old names) assuming we can remove any uses from e.g. Rust and clang.

This comment was removed by aheejin.

Overall looks ok

157 ↗(On Diff #151391)

Why did the the binary encoding change from 0x33 to 0x32 here?

aardappel updated this revision to Diff 151434.Jun 14 2018, 3:39 PM
aardappel marked 3 inline comments as done.

Code review fixes.

15 ↗(On Diff #151391)

Ok, added more description.

157 ↗(On Diff #151391)

I have NO idea how that happened. But thanks for spotting a potentially costly bug. Also we need better tests, apparently.

467 ↗(On Diff #151391)

Ok, removed the FIXMEs for now.

dschuff accepted this revision.Jun 14 2018, 4:26 PM

Otherwise LGTM

39 ↗(On Diff #151434)

I think the "instead implicit" wording is a bit confusing given that we refer to the stack version as the "explicit locals" version. In this comment I might put even more detail like:
"The register versions have virtual-register operands which correspond to wasm locals. Each use and def of the register corresponds to an implicit get_local or set_local operation in wasm. These instructions are used for ISel and all MI passes. The stack versions of the instructions do not have register operands (they implicitly operate on the stack), and get_locals and set_locals are explicit. The register instructions are converted to their corresponding stack instructions before lowering to MC".

This revision is now accepted and ready to land.Jun 14 2018, 4:26 PM
dschuff added inline comments.Jun 14 2018, 4:30 PM
39 ↗(On Diff #151434)

After this lands, it probably makes sense to just turn the explicit-locals pass into the full register->stack conversion pass.

aardappel updated this revision to Diff 151438.Jun 14 2018, 4:49 PM

More code review fixes.

Made final comment fixes.. feel free to land :)

39 ↗(On Diff #151434)

"The register versions have virtual-register operands which correspond to wasm locals": now that I find confusing, since plenty of uses of registers do not correspond to locals.

But you have a point that distinction needs to be made more clear. I'll adapt your text.

39 ↗(On Diff #151434)

I was going to do it just before "WebAssembly Assembly Printer". There are a whole bunch of passes after Explicit Locals below, and I am not sure how easy it is to make them all deal with stack based instructions:

WebAssembly Explicit Locals
WebAssembly CFG Sort
WebAssembly CFG Stackify
WebAssembly Lower br_unless
WebAssembly Register Numbering
Insert fentry calls
Insert XRay ops
Lazy Machine Block Frequency Analysis
Machine Optimization Remark Emitter
WebAssembly Assembly Printer
This revision was automatically updated to reflect the committed changes.