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.
Details
Diff Detail
- Build Status
Buildable 4705 Build 4705: arc lint + arc unit
Event Timeline
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h | ||
---|---|---|
182 | 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? |
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h | ||
---|---|---|
182 | 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? |
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h | ||
---|---|---|
182 | 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. |
LGTM with nit
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp | ||
---|---|---|
155 | This now duplicates WebAssembly::toValType in WebAssemblyMCTargetDesc, can that one be easily used? |
A couple of observations of pre-existing stuff:
- It seems odd that we have both wasm and WebAssembly namespaces
- 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.
- 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.
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?