[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd7086af2143d: [WebAssembly] Support for WebAssembly globals in LLVM IR (authored by pmatos, committed by wingo). Repository: rG LLVM Github Monore

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 344320. wingo added a comment. Rename "managed" address space to "wasm vars" Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 Files: clang/lib/Basic/Targets/WebAssembl

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 sunfish wrote: > tlively w

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 tlively wrote: > sunfish

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 sunfish wrote: > tlively

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 tlively wrote: > sunfish

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast(Op)) +return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + sunfish wrote: > sbc100 wrote: > >

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast(Op)) +return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + sbc100 wrote: > sunfish wrote: >

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast(Op)) +return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + sunfish wrote: > tlively wrote: >

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 sunfish wrote: > Sorry t

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 Sorry to throw more pain

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast(Op)) +return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + tlively wrote: > Actually, should

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast(Op)) +return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + Actually, should we enforce that

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 accepted this revision. sbc100 added a comment. managed seems reasonable yes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 ___ cfe-commits mailing lis

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively accepted this revision. tlively added a comment. This revision is now accepted and ready to land. This LGTM! `MANAGED` sounds like a good address space name to me. @sbc100, do you have any final comments? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 343360. wingo added a comment. yarr, shiver me timbers, fix me typos Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 Files: clang/lib/Basic/Targets/WebAssembly.h cl

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 343344. wingo added a comment. Rename "object" address space to "managed"; address feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 Files: clang/lib/Basic/Ta

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISD.def:48 + +// Reference Types +HANDLE_MEM_NODETYPE(GLOBAL_GET) tlively wrote: > sbc100 wrote: > > Is this just for ref types or also for global that hold integers too (like > > `_

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo marked an inline comment as done. wingo added a comment. Thanks for the review! Regarding the address space name -- the intention with "object" was to rhyme a bit with "object capabilities", somehow, but I see that it is confusing. How about `WASM_ADDRESS_SPACE_MANAGED` ? I.e. this addre

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added a comment. How about `WASM_ADDRESS_SPACE_NON_INTEGRAL` or `WASM_ADDRESS_SPACE_OPAQUE` ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 ___ cfe-com

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; sbc100 wrote: > tlively wrote: > > sbc100 wrote: > > > tlively wrote: > > > > sbc100 wrote: > > > > > tl

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; tlively wrote: > sbc100 wrote: > > tlively wrote: > > > sbc100 wrote: > > > > tlively wrote: > > > > > sb

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; sbc100 wrote: > tlively wrote: > > sbc100 wrote: > > > tlively wrote: > > > > sbc100 wrote: > > > > > Wh

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; tlively wrote: > sbc100 wrote: > > tlively wrote: > > > sbc100 wrote: > > > > What does "object" mean her

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; sbc100 wrote: > tlively wrote: > > sbc100 wrote: > > > What does "object" mean here? Are we just talki

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:40 + // appropriate. + WASM_ADDRESS_SPACE_OBJECT = 1 +}; tlively wrote: > sbc100 wrote: > > What does "object" mean here? Are we just talking about reference ty

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

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

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1378 - if (GA->getAddressSpace() != 0) -fail(DL, DAG, "WebAssembly only expects the 0 address space"); Is it worth checking this is within `WasmAddressSpace`

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sbc100 added a comment. Does this mean that the magic `__stack_pointer` global can be referenced at the IR level? I wonder if there are some hacks around handling of `__stack_pointer` that could be removed after this lands? Comment at: llvm/lib/Target/WebAssembly/Utils/WebA

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 342970. wingo added a comment. Pull in fixes from D101140 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 Files: clang/lib/Basic/Ta

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 342963. wingo marked 2 inline comments as done. wingo added a comment. Address feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 Files: clang/lib/Basic/Targets

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo marked 3 inline comments as done. wingo added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1324 +{LN->getChain(), Base}, LN->getMemoryVT(), LN->getMemOperand()); +return DAG.getMergeValues({GlobalGet, LN->getChain()}, DL);

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 342719. wingo added a comment. Rebase on main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.llvm.org/D101608 Files: clang/lib/Basic/Targets/WebAssembly.h clang/test/CodeGen/target

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

pmatos added a comment. A quick note - apparently phab felt that since I accepted the revision, it's now ready to land. Probably obvious but I accepted expecting @tlively comments to be addressed first. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

pmatos accepted this revision. pmatos added a comment. This revision is now accepted and ready to land. This looks good to me - I will rebase D95425 on this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

tlively added a comment. Nice! I think that splitting this out of the change that adds reference types is a good idea. Regarding the FIXME in the test, is it the case that the globals are also not emitted in the binary format, or is it just the assembly output that is broken? ===

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

wingo updated this revision to Diff 341868. wingo added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fix clang datalayout expectations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101608/new/ https://reviews.ll