Page MenuHomePhabricator

Reland "[WebAssembly] More uses of uint8_t for single byte values"
ClosedPublic

Authored by aheejin on Mar 2 2018, 11:18 AM.

Details

Summary

Original change was D43991 (rL326541) and was reverted by rL326571 and
rL326572. This adds also the necessary MCCodeEmitter patch, thanks to @ncw.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Mar 2 2018, 11:18 AM
sbc100 added inline comments.Mar 2 2018, 11:37 AM
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
97 ↗(On Diff #136813)

There was no assert before and this is no more dangerous so I wouldn't add one as part of this change.

If you must at a new assert the text should maybe say "operand signaure is not a valid wasm value type", and the check should be a range change of valid wasm types.

But I would just drop this assert.

sbc100 accepted this revision.Mar 2 2018, 11:39 AM

I think this change might fix the same issue: D44037

But not reason not to land both.

This revision is now accepted and ready to land.Mar 2 2018, 11:39 AM

These changes make me think we are missing a unit test somewhere

aheejin updated this revision to Diff 136833.Mar 2 2018, 12:53 PM
aheejin marked an inline comment as done.
  • Remove the assertion
This revision was automatically updated to reflect the committed changes.
ncw added a comment.Mar 2 2018, 1:10 PM

LGTM.

The assert came from me, and it was carefully checking the letter and the spirit of the current spec!

I'm not entirely convinced by the change overall - I personally thought it was good to use SLEB before, rather than uint8_t. There's a note in the official spec which makes it very clear "these are really SLEB values in intent", so changing the implementation to uint8_t seems a bit backwards. Yes it matches the official text, but not the official direction of the spec. We're only going to have to put it back to SLEB sometime in the future.

Oh well, it should be a safe change in any case.

sbc100 added a comment.Mar 2 2018, 1:40 PM
In D44034#1025945, @ncw wrote:

LGTM.

The assert came from me, and it was carefully checking the letter and the spirit of the current spec!

I'm not entirely convinced by the change overall - I personally thought it was good to use SLEB before, rather than uint8_t. There's a note in the official spec which makes it very clear "these are really SLEB values in intent", so changing the implementation to uint8_t seems a bit backwards. Yes it matches the official text, but not the official direction of the spec. We're only going to have to put it back to SLEB sometime in the future.

Oh well, it should be a safe change in any case.

I've been making these changes across the board, both in llvm and in wabt. I found it rather strange that the byte constants in the spec did not match those in the code. Also, the spec specificlly says they are single byte values (for now anyway).