This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Use LEB encoding for value types
ClosedPublic

Authored by sbc100 on Mar 10 2017, 2:18 PM.

Details

Summary

Previously we were using the encoded LEB hex values
for the value types. This change uses the decoded
negative value and the LEB encoder to write them out.

Event Timeline

sbc100 created this revision.Mar 10 2017, 2:18 PM
dschuff added inline comments.Mar 10 2017, 3:02 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
182–183

my first comment was here, that it probably makes sense to keep opcodes as unsigned in general; it's the general convention in LLVM and there could eventually be more wasm opcodes than will fit in a uint8.

More generally still, it's pretty common in LLVM to use unsigned for things that will eventually just be encoded (or most things generally really). So the fact that we have a good reason to use negative values here is unusual. So maybe it warrants a typedef instead of just using int32_t?

sbc100 updated this revision to Diff 91426.Mar 10 2017, 3:10 PM

Update types in WebAssemblyMCTargetDesc.h

sbc100 updated this revision to Diff 91427.Mar 10 2017, 3:12 PM
  • Revert change of Nop and End types
sbc100 added inline comments.Mar 10 2017, 3:17 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
182–183

I reverted this part of the change.

I guess we could use the above ValType everywhere in the change rather than int32_t?

Or we could stick with what dan had which is to not decode or encode the negative LEBs at all?

This change was motivated because I'm working other tools which assume that these get decoded and encoded when read and written, but I could instead change my work to use this approach?

dschuff added inline comments.Mar 10 2017, 3:29 PM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
182–183

I do like the idea of using the negative values and encoding them when we encode everything else, rather than using the encoded values everywhere. So the question is whether to use ValType instead of int32_t. I think I'd be inclined to use ValType.

sunfish edited edge metadata.Mar 10 2017, 3:32 PM

Using ValType sounds good to me.

sbc100 updated this revision to Diff 91446.Mar 10 2017, 6:22 PM
  • Move more types to Support/Wasm.h
  • clang-format
dschuff accepted this revision.Mar 13 2017, 8:47 AM

LGTM with nit

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
155

This now duplicates WebAssembly::toValType in WebAssemblyMCTargetDesc, can that one be easily used?

This revision is now accepted and ready to land.Mar 13 2017, 8:47 AM

A couple of observations of pre-existing stuff:

  1. It seems odd that we have both wasm and WebAssembly namespaces
  2. based on the name and the history of wasm, ExprType vs ValType seem like they ought to be the same thing. Right now it looks like ExprType was made for block signatures. We might want to unify or rationalize the use of those somehow; it might not be obvious everywhere on the face of it where we'd need a general block signature (i.e. that may be void) and where we want a non-void type.
sbc100 updated this revision to Diff 91582.Mar 13 2017, 9:59 AM
sbc100 marked an inline comment as done.
  • remove MVT2WasmType in favor of WebAssembly::toValType
  • Merge remote-tracking branch 'refs/remotes/origin/master' into fix_value_types

When wasm gets multiple return values, and maybe block signatures are extended to multiple values, ExprType marks the places that will turn into sequence-of-types, while ValType will always be used for individual values.

In terms the namespacing, yes it probably makes sense to try to merge them and be consistent, but thats not something for this PR.

Ping? Hopefully this change is good to go now.

This revision was automatically updated to reflect the committed changes.