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

[aheejin]

Summary

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

[sbc100]

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

97

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]

I think this change might fix the same issue: D44037

But not reason not to land both.

[sbc100]

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

[aheejin]

• Remove the assertion

[ncw]

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]

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).

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

if (MO.isReg()) {
         const MCOperandInfo &Info = Desc.OpInfo[i];
         if (Info.OperandType == WebAssembly::OPERAND_I32IMM) {
           encodeSLEB128(int32_t(MO.getImm()), OS);
         } else if (Info.OperandType == WebAssembly::OPERAND_I64IMM) {
           encodeSLEB128(int64_t(MO.getImm()), OS);
         } else if (Info.OperandType == WebAssembly::OPERAND_GLOBAL) {
           llvm_unreachable("wasm globals should only be accessed symbolicly");
         } else if (Info.OperandType == WebAssembly::OPERAND_SIGNATURE) {
-          encodeSLEB128(int64_t(MO.getImm()), OS);
+          assert(MO.getImm() > 0 && (MO.getImm() & ~0x3f) == 0x40 &&
+                 "Signature must be pre-encoded negative single-byte SLEB");
+          OS << uint8_t(MO.getImm());
         } else {
           encodeULEB128(uint64_t(MO.getImm()), OS);
         }
       } else {
         assert(Desc.TSFlags == (WebAssemblyII::VariableOpIsImmediate |
                                 WebAssemblyII::VariableOpImmediateIsLabel));
         encodeULEB128(uint64_t(MO.getImm()), OS);
       }

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

 static const unsigned LoadAddressOperandNo = 3;
 static const unsigned StoreAddressOperandNo = 2;
 
 /// The operand number of the load or store p2align in load/store instructions.
 static const unsigned LoadP2AlignOperandNo = 1;
 static const unsigned StoreP2AlignOperandNo = 0;
 
 /// This is used to indicate block signatures.
-enum class ExprType {
-  Void    = -0x40,
-  I32     = -0x01,
-  I64     = -0x02,
-  F32     = -0x03,
-  F64     = -0x04,
-  I8x16   = -0x05,
-  I16x8   = -0x06,
-  I32x4   = -0x07,
-  F32x4   = -0x08,
-  B8x16   = -0x09,
-  B16x8   = -0x0a,
-  B32x4   = -0x0b
+enum class ExprType : unsigned {
+  Void    = 0x40,
+  I32     = 0x7F,
+  I64     = 0x7E,
+  F32     = 0x7D,
+  F64     = 0x7C,
+  I8x16   = 0x7B,
+  I16x8   = 0x7A,
+  I32x4   = 0x79,
+  F32x4   = 0x78,
+  B8x16   = 0x77,
+  B16x8   = 0x76,
+  B32x4   = 0x75
 };
 
 /// Instruction opcodes emitted via means other than CodeGen.
 static const unsigned Nop = 0x01;
 static const unsigned End = 0x0b;
 
 wasm::ValType toValType(const MVT &Ty);
 
 } // end namespace WebAssembly
 } // end namespace llvm
 
 #endif