tlively added a comment. LGTM with Sam's comments resolved!
================ Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; ---------------- sbc100 wrote: > What does "object" mean here? Are we just talking about reference types? > Or also wasm globals that hold integers (like `__stack_pointer`). If its > just ref types that live in this address space should this be called > `WASM_ADDRESS_SPACE_ANYREF`? If its the latter should this be called > `WASM_ADDRESS_SPACE_WASM_GLOBAL`? I was also wondering about the best name here because OBJECT is somewhat vague. I think the idea is that this address space can be used for arbitrary Wasm globals of any type, but it could also be used later for things like additional tables and memories. It's unclear whether those would need separate address spaces for some reason, but if they don't, re-using this address space 1 would be best. Maybe `WASM_ADDRESS_SPACE_STATIC` would be a better name because it will be used for things that are given static indices in the final module? ================ Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISD.def:48 + +// Reference Types +HANDLE_MEM_NODETYPE(GLOBAL_GET) ---------------- sbc100 wrote: > Is this just for ref types or also for global that hold integers too (like > `__stack_pointer`) Yep, it's for arbitrary types (see the tests), so this comment should be updated. ================ Comment at: llvm/test/CodeGen/WebAssembly/global-get.ll:13 +; CHECK-NEXT: end_function + %v = load i32, i32 addrspace(1)* @i32_global + ret i32 %v ---------------- sbc100 wrote: > I don't suppose there is any way to give `addrspace(1)` a symbolic name is > there? Not that I know of, unfortunately :( Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
