Just a little thing I saw while reading code -- if no response in a couple weeks I will just abandon the rev

@Anastasia is this good to go for you or does this need closer attention?

Thanks again John & Thomas for your thoughts.

Closing PR as it turns out that I don't need the Address() change for my work; no need to churn here.

In D108458#2957789, @rjmccall wrote:

You can still use a type without a default constructor in a DenseMap; you just have to use insert instead of dict[key] = ....

Rebase to no longer require Address default constructor.

Thanks for feedback! Following up on general question in D108464.

In D108464#2957276, @wingo wrote:

Sooooo... besides the refactor, this is getting closer to where I'm going in https://lists.llvm.org/pipermail/cfe-dev/2021-July/068559.html, though still NFC. I think you can see where I would replace getASTAllocaAddressSpace with getAllocaAddressSpace(QualType Ty), and possibly (depending on the source language) avoid casting the resulting alloca to LangAS::Default. WDYT, is this sort of thing OK?

Sooooo... besides the refactor, this is getting closer to where I'm going in https://lists.llvm.org/pipermail/cfe-dev/2021-July/068559.html, though still NFC. I think you can see where I would replace getASTAllocaAddressSpace with getAllocaAddressSpace(QualType Ty), and possibly (depending on the source language) avoid casting the resulting alloca to LangAS::Default. WDYT, is this sort of thing OK?

I tested by building with all targets enabled and there was no change in test results, running clang, llvm, and lld tests.

Just a drive-by cleanup while poking other address-space-related things.

Hi John, I am new to clang, having mostly worked on the WebAssembly target in llvm proper. I have a stack of patches related to address space treatment in codegen -- most of them NFC refactors to later allow the WebAssembly target to alloca in different address spaces. See https://lists.llvm.org/pipermail/cfe-dev/2021-July/068559.html for broader context. Would it be OK if I sent them your way? Cheers, Andy.

Hi Richard, I am new to clang, having mostly worked on the WebAssembly target in llvm proper -- I have a stack of patches, of which a couple later ones touch Sema/. See https://lists.llvm.org/pipermail/cfe-dev/2021-July/068559.html for broader context. However I had a couple small NFC cleanups I found when reading code, like this one; would it be OK if I sent them your way? Cheers, Andy.

Neat :) LGTM as well of course.

Prettify tests

Tighten up tests

Thanks all for the feedback! I think the updated patch addresses the feedback; PTAL :)

Use STT_NOTYPE for synthesized symbols

Add wasm symbols for any function without a symbol

Rebase; will fix spurious presubmit errors

Simplify test

Quick r? to @tlively -- OK with having the new util function in WebAssemblyFrameLowering? It didn't work to have it in the utils library because the function needs the vtable of WebAssemblyFunctionInfo.

Fix shared-library build by moving util function to WebAssemblyFrameLowering.

Yarrrgh, broke the shared library build. Fix incoming...

In D101140#2786870, @jrtc27 wrote:

In D101140#2786844, @wingo wrote:

In D101140#2786777, @jrtc27 wrote:

Is it just me or does having a backend-specific type in target-independent code feel wrong? It feels like there should be a space for target-specific TargetStackIDs...

Hoo, good question. More support for target-specific handling of stack IDs would be great. However in this case the concept is not purely wasm-specific; I can imagine other targets that might have a similar treatment of locals (if we had a .net target, or a jvm target, or so). The idea is that there is a separate stack consisting of named locals that may not be addressable by pointers to main memory. In earlier drafts of this patch the name was more generic ("Object", then "Managed") but you know, our words in this area are quite overloaded. So instead I went with something quite specific (WasmLocal) to avoid the general question -- but I do think the concept is not specific, even if it doesn't apply to any other target currently in tree. WDYT?

Well, except all the logic for it is in the backend, only the parser and the definition are in target-independent code, so even if another backend were to reuse it it would have its own completely separate logic for it, and thus there's no benefit to reusing a shared name for the thing over each target defining its own? Or would some of the code in the wasm backend be refactored out into CodeGen?

In D101140#2786777, @jrtc27 wrote:

Is it just me or does having a backend-specific type in target-independent code feel wrong? It feels like there should be a space for target-specific TargetStackIDs...

Fix build for RISC-V and AMDGPU.

Adapt test to insert compiler barrier.

Gentle ping here to @tlively :)

Address feedback

Rebase

Remove AMDGPU test that alloca in wrong addrspace fails to verify

Tweak alloca langref language

Fix drop-debug-info-nonzero-alloca test

Go back to first version of patch, removing verifier restriction on alloca address spaces

In D101045#2755666, @arsenm wrote:

In D101045#2718774, @wingo wrote:

In D101045#2711957, @arsenm wrote:

It hadn't occurred to me to add a field to the datalayout purely for the verifier. That seems a bit strange, since it's not exactly a target property.

Yeah understood. If we want some targets to have the check and some to not have the check, do you know of a better place to express this? I thought the same place that the target's alloca address space is defined might not be unreasonable, but I am new here :)

I think we should have target specific IR verifiers, not pollute the datalayout. There are a number of other places such a verifier would be useful

Rename "managed" address space to "wasm var".

Update in response to parent commit changes

Rename "managed" address space to "wasm vars"

Fix spurious whitespace

Address feedback from tlively

Rename "object" to "managed", and address feedback

Rename "object" to "managed", and add test for stack id

yarr, shiver me timbers, fix me typos

Rename "object" address space to "managed"; address feedback.