[PATCH] D104945: [WebAssembly] Added initial type checker to MC Assembler

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 8 inline comments as done.
aardappel added inline comments.


================
Comment at: lld/test/wasm/bsymbolic.s:102
 
+.globl bar
 .globl foo
----------------
sbc100 wrote:
> Is this change needed here or just for consistency?    If its needed why does declaring the linkage type as global up front help the validator?
not needed, probably part of an earlier refactor I changed later. reverted.


================
Comment at: llvm/test/CodeGen/WebAssembly/global-get.ll:76
 ; CHECK-NOT: .global i32_external_used
-; CHECK: .globaltype i32_external_used, i32
+; CHECK-NOT: .globaltype i32_external_used, i32
 ; CHECK-NOT: i32_external_used:
----------------
sbc100 wrote:
> This seems like i32_external_used should have some kind of type, no?    I guess we should look at the FIXME here before landing.
Yes, this is the case I mentioned I couldn't figure out. This seems like the only place where we are declaring custom globals and they are handled differently from the standard ones it seems..


================
Comment at: llvm/test/MC/WebAssembly/basic-assembly.s:59
-    block       f32
-    block       f64
     block       () -> (i32, i32)
----------------
sbc100 wrote:
> Did we not loose coverage of these different block types here?
This file is really a random collection of wasm features, meant to test the assembler more than the features in question. This file does not promise to be complete in any way.. I simply made the changes in a way to require as few changes to the original as possible while type-checking.


================
Comment at: llvm/test/MC/WebAssembly/basic-assembly.s:114
     global.get  __stack_pointer
+    global.set  __stack_pointer
     end_function
----------------
sbc100 wrote:
> Why this?  Why not just drop?
I guess I was testing global.set as to how its handled in the type checker? drop would allow any type from global.get, so not a great test. Again, this file is a grab-bag of tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104945/new/

https://reviews.llvm.org/D104945