This patch introduces a new type externref_t that denotes a WebAssembly opaque
reference type. It also implements builtin builtin_wasm_ref_null_extern(),
that returns a null value of __externref_t. This lays the ground work
for further builtins and reference types.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Incorporate @asb semantic restrictions patch.
Generate test CHECK using update_cc_* script.
It would be great to get this landed. Can someone pls take a look at it? It's quite independent from the remainder of the work and self-contained.
maybe @tlively ping?
Very nice! This LGTM with all these small comments addressed. Sorry for the long delay in reviewing.
clang/include/clang/AST/ASTContext.h | ||
---|---|---|
1158 | I have no idea what's going on here in the code, but it seems that the existing convention is to put CanQualType SingletonId; on a separate line. | |
clang/include/clang/AST/Type.h | ||
1976 | Looks like there should be a newline here. | |
clang/include/clang/Basic/AddressSpaces.h | ||
59–60 | What is wasm_var? It would be good to have a short comment here (and newline afterward). | |
clang/include/clang/Basic/Builtins.def | ||
50 ↗ | (On Diff #446405) | |
clang/include/clang/Basic/BuiltinsWebAssembly.def | ||
194 | Looks like this comment about polymorphism is out of date. (Also it would be good to add a newline after this.) | |
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
10 | Maybe we should only mention externref_t for now. | |
clang/lib/AST/ASTContext.cpp | ||
2261 | I assume things will break if you say 0 here, but would 1 work? | |
3990 | Do we need Target.getTriple().isWasm() here as well? | |
clang/lib/AST/ItaniumMangle.cpp | ||
3147 | Our MangledName is not the same as our InternalName, so it looks like this condition will never be true. Should be follow the simpler pattern from the previous two targets instead? | |
clang/lib/Basic/Targets/DirectX.h | ||
44–45 | What are these for? I'm surprised we need to do anything here in the DirectX target. Same for the similar lines in other targets. | |
clang/lib/CodeGen/CGDebugInfo.cpp | ||
809–810 | How did you choose this? | |
clang/lib/CodeGen/TargetInfo.h | ||
366 | missing whitespace. | |
clang/test/CodeGen/WebAssembly/wasm-externref.c | ||
12–14 | Do we need this here since the builtin is also tested in builtins-wasm.c? Are there more ways to use externref_t that we should test here? | |
clang/test/Sema/wasm-refs-and-tables.c | ||
4 | Should this file be just wasm-refs.c since it doesn't do anything with tables yet? Same for the next one. | |
clang/test/SemaTemplate/address_space-dependent.cpp | ||
46 | Maybe this test can be split off into a separate tiny PR since it doesn't look directly related to the rest. |
Adding new types to the type system is quite invasive; was there an RFC for this you can point me to along with a design document? I have no idea how to review this because I don't know what the semantics of this type are supposed to be.
clang/include/clang/AST/ASTContext.h | ||
---|---|---|
1506 | ||
clang/include/clang/AST/Type.h | ||
1975–1976 | It's unfortunate to name this with ReferenceType given that we already have a considerable number of APIs that assume "reference type" to mean & or &&. We run into similar problems with "pointer type" and objective-c pointers. Basically, I worry we're setting ourselves up for another isObjCObjectPointerType()/isPointerType() situation. | |
clang/include/clang/Basic/AddressSpaces.h | ||
59–60 | Agreed; I'm not certain why wasm_var is part of this changeset. | |
clang/include/clang/Basic/Attr.td | ||
3937–3944 ↗ | (On Diff #417212) | No new undocumented attributes, please. No need for the Args field, and please add the newline back to the end of the file. |
clang/include/clang/Basic/Builtins.def | ||
50 ↗ | (On Diff #446405) | How often do you expect to use this marking? If it's only going to be used once or twice, it might make sense to not steal a letter but instead use custom checking for the signature. |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
11674–11677 | These can be combined with a %select | |
11678–11681 | These can also be combined with a %select | |
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
17 | What kind of mangling is this name? Itanium? Microsoft? Something else? | |
clang/lib/AST/ASTContext.cpp | ||
955–956 | How did you arrive at these values? | |
clang/lib/AST/ItaniumMangle.cpp | ||
3147 | I'm not an Itanium mangling expert, but doesn't this *have* to use the u marking per https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangle.builtin-type because this is definitely a vendor extension type. | |
clang/lib/AST/MicrosoftMangle.cpp | ||
2484–2485 | Is it reasonable that this simply cannot mangle in Microsoft ABI mode? | |
clang/lib/AST/Type.cpp | ||
2330–2333 | ||
clang/lib/Sema/SemaType.cpp | ||
2195–2200 | ||
2278–2283 |
clang/include/clang/AST/Type.h | ||
---|---|---|
1975–1976 | I understand this concern. However, unsure what else to call it given that's what it is. It's a WebAssembly Reference Type, which indeed is different from a C/C++ ReferenceType. Could call it OpaqueType but that would be even worse. Not only it's not called an opaque type in Wasm, it's also not what LLVM users will think of as Opaque Types. | |
clang/include/clang/Basic/Builtins.def | ||
50 ↗ | (On Diff #446405) | Ah yes, that probably makes sense. I will use a custom typechecker instead. |
clang/include/clang/AST/Type.h | ||
---|---|---|
1975–1976 | Yeah, I think this is as good of a name as we're likely to find; I certainly haven't thought of something better. If we do think of something later, we can refactor then. |
clang/include/clang/AST/Type.h | ||
---|---|---|
1975–1976 | OK - thanks. |
clang/include/clang/Basic/AddressSpaces.h | ||
---|---|---|
59–60 | Yes, it was a mistake during patch splitting. I have removed it. Thanks. | |
clang/lib/AST/ASTContext.cpp | ||
2261 | Yes, 1 seems to work. I am not 100% sure but since externref cannot be written to memory, does it matter? | |
3990 | Might be useful, in case another target has also a feature called reference-types. :) | |
clang/lib/Basic/Targets/DirectX.h | ||
44–45 | I was surprised as well, but apparently if you don't add them to all targets, they will not compile. There's an assignment of these Target specific AddrSpaceMaps to the AddrSpaceMap: AddrSpaceMap = &DirectXAddrSpaceMap;, and AddrSpaceMap is a LangAS defined in AddressSpaces.h so anything added there needs to be added to all targets afaiu. | |
clang/test/Sema/wasm-refs-and-tables.c | ||
4 | Tables to be added in a future patch. ;) |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
955–956 |
Same as for funcref, this is the same in the LLVM side of things. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
2261 | Nope... I take that back. builtins-wasm.c seems to break with anything under 8 with the error: clang: /home/pmatos/dev/llvm-project/llvm/include/llvm/Support/Alignment.h:77: llvm::Align::Align(uint64_t): Assertion `Value > 0 && "Value must not be 0"' failed. Still need to investigate why a value of Align = 1; here ends up causing a zero alignment value in Alignment.h. |
Current patch addresses most of the concerns - also passed through ./clang/tools/clang-format/git-clang-format.
Ready for review.
RFC: https://discourse.llvm.org/t/rfc-webassembly-reference-types-in-clang/66939
Thanks for the patch! I just tried it out and I think this enables many interesting use cases for WebAssembly when using C/C++.
Currently the lowering to wasm does not work when using externrefs when compiling without optimizations. Is that intended behavior?
I have this example C++ program (similar to the wasm-externref.c test):
void extern_func(__externref_t ref); void my_func(__externref_t ref) { extern_func(ref); }
If I compile this without optimizations (exact command: clang++ --target=wasm32 -mreference-types -c ./test_externref.cpp), I get the following error:
fatal error: error in backend: Cannot select: 0x621000066bd0: externref,ch = load<(dereferenceable load (s0) from %ir.2)> 0x621000066b58, TargetFrameIndex:i32<0>, undef:i32 0x621000066a68: i32 = TargetFrameIndex<0> 0x621000066ae0: i32 = undef
The reason for that can be seen when we look at the generated IR for this program:
; Function Attrs: mustprogress noinline optnone define hidden void @_Z7my_func11externref_t(ptr addrspace(10) %0) #0 { %2 = alloca ptr addrspace(10), align 1 store ptr addrspace(10) %0, ptr %2, align 1 %3 = load ptr addrspace(10), ptr %2, align 1 call void @_Z11extern_func11externref_t(ptr addrspace(10) %3) ret void } declare void @_Z11extern_func11externref_t(ptr addrspace(10)) #1
Apparently when no optimizations are enabled, clang will write all function arguments to an alloca'd variable. This obviously won't work if the value is an externref. The test case in wasm-externref.c also has the alloca and store with an externref value as expected output, which I think should never be generated.
With -O3 this doesn't happen as the alloca'd variables are optimized out. But I still think it should also work with -O0.
No - thanks for trying this out. This is definitely not intended behaviour. Somehow I probably didn't test enough without optimizations. I was expecting that the pass mem2reg would get rid of the allocas, but maybe it's not running at those optimization levels. I will have to take a closer look.
Apparently the code forcing mem2reg was lost during a conflict resolution. My apologies, I will add this to the patch asap.
I have posted a new patch that fixed this. Can you please try it again and see if it works as you expect?
Thanks for the quick update, it works now!
Does this affect the debugging experience of the generated wasm programs? If you force mem2reg to run for all programs, no function arguments will be written to the "stack" referenced by the __stack_pointer global variable anymore. I don't know whether any wasm runtime actually uses that for debugging, though.
clang/include/clang/AST/ASTContext.h | ||
---|---|---|
1507 | getWebAssemblyExternrefType ? | |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
11675 | Rewording to avoid using "illegal" (and changes to the singular form). | |
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
17 | Still wondering about this. | |
clang/lib/AST/ASTContext.cpp | ||
3990–3996 | Rather than returning a null type, should we assert we're in a wasm triple? We shouldn't be trying to form the type outside of a wasm target, right? | |
clang/lib/AST/ItaniumMangle.cpp | ||
3147 | Still wondering about this, pinging @rjmccall for Itanium ABI expertise | |
clang/lib/AST/MicrosoftMangle.cpp | ||
2484–2485 | Still wondering about this |
Deal with Itanium Mangle and assert false outside Wasm triple.
clang/lib/AST/MicrosoftMangle.cpp | ||
---|---|---|
2484–2485 | @aaron.ballman Why wouldn't externref_t be able to mangle in Microsoft ABI mode? Quite clueless when it comes to ABIs. |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
3990–3996 | I'd recommend something like: assert(Target->getTriple().isWasm() && Target->hasFeature("reference-types") && "shouldn't try to generate type externref outsid of WebAssembly target"); #define WASM_REF_TYPE(Name, MangledName, Id, SingletonId, AS) \ if (BuiltinType::Id == BuiltinType::WasmExternRef) \ return SingletonId; #include "clang/Basic/WebAssemblyReferenceTypes.def" } llvm_unreachable("couldn't find the externref type?"); so it's clear that you shouldn't call this outside of a wasm target and that it's not an option to not find the type. | |
clang/lib/AST/MicrosoftMangle.cpp | ||
2484–2485 | Including the wasm types here means that in Microsoft mode, use of those types in a context which requires mangling will diagnose (see this snippet just below): unsigned DiagID = Diags.getCustomDiagID( DiagnosticsEngine::Error, "cannot mangle this built-in %0 type yet"); Diags.Report(Range.getBegin(), DiagID) << T->getName(Context.getASTContext().getPrintingPolicy()) << Range; So I'm wondering whether you consider that to be reasonable behavior or not. If you don't want the diagnostic, then you should add a vendor extension mangling for the type. |
clang/lib/CodeGen/CGDebugInfo.cpp | ||
---|---|---|
809–810 | This is the creation of the debug info for an opaque structure type, although I guess we could use other representations for externref, do you have any suggestions? | |
clang/test/CodeGen/WebAssembly/wasm-externref.c | ||
12–14 | Not at the moment. There's not much you can do with externref without using tables for example which is in another patch. Happy to remove this test as we have something similar indeed in builtins-wasm.c. |
@aaron.ballman many thanks for the thorough reviews on the patches. Happy to see this moving in the right direction.
clang/lib/AST/MicrosoftMangle.cpp | ||
---|---|---|
2484–2485 | Oh - now I understand your point. I had forgotten I had initially written this 'workaround' to worry with later. Yes, let me add a vendor extension mangling for Microsoft ABI. |
Now under the correct revision: @aaron.ballman Added support for externref mangling in Microsoft. This follows the pattern implemented by OpenCL types.
I feel like the latest patch addresses all concerns until now. What do you think?
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
---|---|---|
17 | This is just a generic name to base the actual mangling on tbh. |
Aside from a nit about comments, I don't have any further concerns. LGTM!
clang/include/clang/Basic/WebAssemblyReferenceTypes.def | ||
---|---|---|
17 | Can you clarify the comment then? I'd expect MangledName to be an actual mangling, otherwise, and that's why I was wondering "which mangling?". |
clang/lib/AST/ASTContext.cpp | ||
---|---|---|
3990–3996 | In a non-assert build, doing just assert(false) causes the build to fail if you use -Werror -Wreturn-type. clang/lib/AST/ASTContext.cpp:4097:1: error: non-void function does not return a value in all control paths [-Werror,-Wreturn-type] |
@pmatos and I have tried and failed to reproduce the assert in the stage3 msan build locally (both of us on separate machines, different environments). We've reached out to @vitalybuka via email for any help in reproducing, because we're somewhat stumped at the moment.
I have no idea what's going on here in the code, but it seems that the existing convention is to put CanQualType SingletonId; on a separate line.