This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Initial support for reference type externref in clang
ClosedPublic

Authored by pmatos on Mar 22 2022, 1:48 AM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
asb added a comment.Jun 20 2022, 6:17 AM

I've left one minor comment. My other suggestion would be to absorb in the semantic restrictions work from here.

llvm/lib/IR/Type.cpp
309

With the move to opaque pointers, I think this and getWasm_FuncRefTy could be updated to just return a pointer to addrspace 10 and 20 respectively?

pmatos updated this revision to Diff 438622.Jun 21 2022, 3:12 AM

Incorporate @asb semantic restrictions patch.
Generate test CHECK using update_cc_* script.

pmatos updated this revision to Diff 438722.Jun 21 2022, 8:42 AM

rebase on top of D128282

pmatos updated this revision to Diff 445490.Jul 18 2022, 7:28 AM

Updated patch over current main. Fixed all failint tests. Ready for review.

pmatos updated this revision to Diff 445493.Jul 18 2022, 7:35 AM

Remove some extraneous spaces I hadn't noticed before.

pmatos updated this revision to Diff 446405.Jul 21 2022, 2:57 AM

Rebase on top of main.

pmatos added a comment.Aug 5 2022, 6:28 AM

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
1129

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
2031

Looks like there should be a newline here.

clang/include/clang/Basic/AddressSpaces.h
59–60 ↗(On Diff #446405)

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
2267

I assume things will break if you say 0 here, but would 1 work?

4012

Do we need Target.getTriple().isWasm() here as well?

clang/lib/AST/ItaniumMangle.cpp
3143

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 ↗(On Diff #446405)

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
825–826

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.

aaron.ballman requested changes to this revision.Aug 17 2022, 6:55 AM

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
1479
clang/include/clang/AST/Type.h
2030–2031

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 ↗(On Diff #446405)

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
11763–11766

These can be combined with a %select

11767–11770

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
933–934

How did you arrive at these values?

clang/lib/AST/ItaniumMangle.cpp
3143

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
2494–2495

Is it reasonable that this simply cannot mangle in Microsoft ABI mode?

clang/lib/AST/Type.cpp
2353–2356
clang/lib/Sema/SemaType.cpp
2200–2205
2282–2287
This revision now requires changes to proceed.Aug 17 2022, 6:55 AM
pmatos marked 10 inline comments as done.Sep 12 2022, 11:57 PM
pmatos added inline comments.
clang/include/clang/AST/Type.h
2030–2031

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.

aaron.ballman added inline comments.Sep 15 2022, 9:53 AM
clang/include/clang/AST/Type.h
2030–2031

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.

pmatos updated this revision to Diff 460696.Sep 16 2022, 3:49 AM
pmatos marked an inline comment as done.

Fix a few of the issues mentioned in the revision comments.

pmatos planned changes to this revision.Sep 16 2022, 3:50 AM

Still a few more changes are required.

pmatos marked 2 inline comments as done.Sep 16 2022, 4:03 AM
pmatos added inline comments.
clang/include/clang/AST/Type.h
2030–2031

OK - thanks.

pmatos marked 10 inline comments as done.Sep 16 2022, 5:00 AM
pmatos added inline comments.
clang/include/clang/Basic/AddressSpaces.h
59–60 ↗(On Diff #446405)

Yes, it was a mistake during patch splitting. I have removed it. Thanks.

clang/lib/AST/ASTContext.cpp
2267

Yes, 1 seems to work. I am not 100% sure but since externref cannot be written to memory, does it matter?

4012

Might be useful, in case another target has also a feature called reference-types. :)

clang/lib/Basic/Targets/DirectX.h
44–45 ↗(On Diff #446405)

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. ;)
But yeah, I will rename for now.

pmatos updated this revision to Diff 460706.Sep 16 2022, 5:53 AM
pmatos marked 4 inline comments as done.

Further changes - more to come.

pmatos planned changes to this revision.Sep 16 2022, 5:53 AM
pmatos marked an inline comment as done.Sep 18 2022, 11:58 PM
pmatos added inline comments.
clang/lib/AST/ASTContext.cpp
933–934

How did you arrive at these values?

Same as for funcref, this is the same in the LLVM side of things.

pmatos marked an inline comment as done.Sep 20 2022, 12:03 AM
pmatos added inline comments.
clang/lib/AST/ASTContext.cpp
2267

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.

pmatos updated this revision to Diff 461518.Sep 20 2022, 2:19 AM

Address further concerns.

pmatos planned changes to this revision.Sep 20 2022, 2:19 AM

Current patch addresses most of the concerns - also passed through ./clang/tools/clang-format/git-clang-format.

pmatos updated this revision to Diff 470517.Oct 25 2022, 8:59 AM

Removed address space wasm_externref which is unnecessary with current design.

pmatos planned changes to this revision.Oct 25 2022, 9:00 AM

Whole integration plan still in the works, so marking as planning changes.

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.

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?

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.

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?

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.

pmatos updated this revision to Diff 483203.Dec 15 2022, 8:36 AM

Force mem2reg on all optimization levels to avoid allocas.

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 posted a new patch that fixed this. Can you please try it again and see if it works as you expect?

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 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.

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 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.

That's a good point, but I don't think that's a large concern at the moment on the WebAssembly side. I wonder if @tlively or @sbc100 have more to add with respect to the current state of debugging Wasm programs nowadays.

koplas added a subscriber: koplas.Jan 9 2023, 4:09 AM
pmatos updated this revision to Diff 487693.Jan 9 2023, 11:12 PM

Rebasing on top of main.

aaron.ballman added inline comments.
clang/include/clang/AST/ASTContext.h
1480

getWebAssemblyExternrefType ?

clang/include/clang/Basic/DiagnosticSemaKinds.td
11778

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
4012–4018

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
3143

Still wondering about this, pinging @rjmccall for Itanium ABI expertise

clang/lib/AST/MicrosoftMangle.cpp
2494–2495

Still wondering about this

pmatos marked 2 inline comments as done.Jan 12 2023, 4:08 AM

Addressed a few comments downstream. Will upload new patch next.

pmatos updated this revision to Diff 488598.Jan 12 2023, 4:25 AM

Address a couple of comments by @aaron.ballman

pmatos marked 5 inline comments as done.Jan 12 2023, 4:45 AM
pmatos added inline comments.
clang/lib/AST/ASTContext.cpp
4012–4018

asserting false with message rather than llvm_unreachable is the right way, correct?

clang/lib/AST/ItaniumMangle.cpp
3143

I think what you both are saying makes sense.

pmatos updated this revision to Diff 488609.Jan 12 2023, 5:10 AM
pmatos marked an inline comment as done.

Deal with Itanium Mangle and assert false outside Wasm triple.

clang/lib/AST/MicrosoftMangle.cpp
2494–2495

@aaron.ballman Why wouldn't externref_t be able to mangle in Microsoft ABI mode? Quite clueless when it comes to ABIs.

aaron.ballman added inline comments.Jan 12 2023, 6:21 AM
clang/lib/AST/ASTContext.cpp
4012–4018

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
2494–2495

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.

pmatos marked an inline comment as done.Jan 12 2023, 6:22 AM
pmatos added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
825–826

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.

pmatos updated this revision to Diff 488634.Jan 12 2023, 6:23 AM
pmatos marked an inline comment as done.

Address all comments except a couple of issues related to ABI and mangling.

pmatos marked an inline comment as done.Jan 12 2023, 6:39 AM

@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
2494–2495

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.

pmatos updated this revision to Diff 488648.Jan 12 2023, 7:41 AM

Now under the correct revision: @aaron.ballman Added support for externref mangling in Microsoft. This follows the pattern implemented by OpenCL types.

aaron.ballman added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
2479–2483

I don't think the PA bit is correct -- that seems to be specific to OpenCL. So I think that bit can simply be dropped, but CC @rnk for questions about Microsoft ABI.

pmatos added inline comments.Jan 12 2023, 11:46 PM
clang/lib/AST/MicrosoftMangle.cpp
2479–2483

OK, that makes sense. Although I got the feeling PA stood for Pointer or similar. Maybe @rnk can clarify. I will follow your suggestion for now.

pmatos updated this revision to Diff 488881.Jan 12 2023, 11:48 PM

Update mangling for Microsoft.

pmatos updated this revision to Diff 488999.Jan 13 2023, 7:20 AM

Fix tests after changing name mangling and error messages.

pmatos updated this revision to Diff 489005.Jan 13 2023, 7:38 AM

Rename test that still had tables in its name.

pmatos updated this revision to Diff 489520.Jan 16 2023, 5:49 AM
pmatos marked 7 inline comments as done.

Rebase on main.

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.

pmatos updated this revision to Diff 493226.Jan 30 2023, 1:04 AM

Rebase on main.

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?".

aaron.ballman accepted this revision.Jan 31 2023, 6:37 AM

Oops, it helps to actually accept it in Phab. :-D

This revision is now accepted and ready to land.Jan 31 2023, 6:37 AM
pmatos updated this revision to Diff 493633.Jan 31 2023, 8:28 AM

Address last comment before landing.

This revision was landed with ongoing or failed builds.Jan 31 2023, 8:35 AM
This revision was automatically updated to reflect the committed changes.
jgorbe added a subscriber: jgorbe.Jan 31 2023, 9:30 AM
jgorbe added inline comments.
clang/lib/AST/ASTContext.cpp
4012–4018

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]
vitalybuka reopened this revision.Feb 5 2023, 9:47 PM
This revision is now accepted and ready to land.Feb 5 2023, 9:47 PM
vitalybuka updated this revision to Diff 494999.Feb 5 2023, 9:48 PM
vitalybuka added a subscriber: vitalybuka.

as reverted

asb added a comment.Feb 10 2023, 8:44 AM

@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.

@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 will try to reproduce on fresh VM and reply to you with details.

asb added a comment.Feb 10 2023, 11:10 AM

@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 will try to reproduce on fresh VM and reply to you with details.

Thanks Vitaly - that would be really helpful.

pmatos updated this revision to Diff 497630.Feb 15 2023, 4:20 AM

Rebased against HEAD. Waiting for @vitalybuka go ahead to land.

clang-format

This revision was landed with ongoing or failed builds.Feb 17 2023, 6:49 PM
This revision was automatically updated to reflect the committed changes.