Page MenuHomePhabricator

[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
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
1481

getWebAssemblyExternrefType ?

clang/include/clang/Basic/DiagnosticSemaKinds.td
11802

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
4090–4096

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
3237

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

clang/lib/AST/MicrosoftMangle.cpp
2499–2500

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
4090–4096

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

clang/lib/AST/ItaniumMangle.cpp
3237

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
2499–2500

@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
4090–4096

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
2499–2500

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
2499–2500

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
2484–2488

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
2484–2488

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
4090–4096

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.