Details

Reviewers

tlively
asb
shafik
aaron.ballman

Commits

rG890146b19206: [WebAssembly] Initial support for reference type externref in clang
rGeb66833d1957: [clang][WebAssembly] Initial support for reference type externref in clang

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

Repository: rG LLVM Github Monorepo

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Force mem2reg on all optimization levels to avoid allocas.

In D122215#3991648, @MoritzS wrote:

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?

Harbormaster completed remote builds in B203366: Diff 483203.Dec 15 2022, 11:00 AM

In D122215#3998105, @pmatos wrote:

In D122215#3991648, @MoritzS wrote:

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.

In D122215#4000612, @MoritzS wrote:

In D122215#3998105, @pmatos wrote:

In D122215#3991648, @MoritzS wrote:

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

Rebasing on top of main.

Harbormaster completed remote builds in B206719: Diff 487693.Jan 10 2023, 12:28 AM

aaron.ballman added a subscriber: rjmccall.Jan 11 2023, 7:40 AM

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

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

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.

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.

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

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

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 a subscriber: rnk.Jan 12 2023, 8:20 AM

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.

Harbormaster completed remote builds in B207402: Diff 488648.Jan 12 2023, 10:28 AM

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.

Update mangling for Microsoft.

Harbormaster completed remote builds in B207554: Diff 488881.Jan 13 2023, 12:35 AM

Fix tests after changing name mangling and error messages.

Rename test that still had tables in its name.

Harbormaster completed remote builds in B207643: Diff 489005.Jan 13 2023, 8:52 AM

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.

Harbormaster completed remote builds in B208030: Diff 489520.Jan 16 2023, 7:06 AM

Rebase on main.

Harbormaster completed remote builds in B210713: Diff 493226.Jan 30 2023, 2:53 AM

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

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

Address last comment before landing.

This revision was landed with ongoing or failed builds.Jan 31 2023, 8:35 AM

Closed by commit rGeb66833d1957: [clang][WebAssembly] Initial support for reference type externref in clang (authored by pmatos). · Explain Why

This revision was automatically updated to reflect the committed changes.

pmatos added a commit: rGeb66833d1957: [clang][WebAssembly] Initial support for reference type externref in clang.

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]

Harbormaster completed remote builds in B211009: Diff 493633.Jan 31 2023, 10:00 AM

vitalybuka added a reverting change: rGbccf5999d38f: Revert "[clang][WebAssembly] Initial support for reference type externref in….Feb 5 2023, 9:45 PM

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

as reverted

Harbormaster completed remote builds in B212000: Diff 494999.Feb 5 2023, 10:34 PM

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

In D122215#4118516, @asb wrote:

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

In D122215#4119038, @vitalybuka wrote:

In D122215#4118516, @asb wrote:

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

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

Harbormaster completed remote builds in B213864: Diff 497630.Feb 15 2023, 5:39 AM

vitalybuka mentioned this in D144316: [SCEV] Fix FoldID::addInteger(unsigned long I).Feb 17 2023, 6:32 PM

clang-format

This revision was landed with ongoing or failed builds.Feb 17 2023, 6:49 PM

Closed by commit rG890146b19206: [WebAssembly] Initial support for reference type externref in clang (authored by pmatos, committed by vitalybuka). · Explain Why

This revision was automatically updated to reflect the committed changes.

vitalybuka added a commit: rG890146b19206: [WebAssembly] Initial support for reference type externref in clang.

Harbormaster completed remote builds in B214544: Diff 498550.Feb 17 2023, 8:21 PM

vitalybuka mentioned this in rGa53d940cee6f: [SCEV] Fix FoldID::addInteger(unsigned long I).Feb 18 2023, 12:29 PM

[WebAssembly] Initial support for reference type externref in clangClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 498553

clang/include/clang/AST/ASTContext.h

clang/include/clang/AST/Type.h

clang/include/clang/AST/TypeProperties.td

clang/include/clang/Basic/BuiltinsWebAssembly.def

clang/include/clang/Basic/DiagnosticSemaKinds.td

clang/include/clang/Basic/WebAssemblyReferenceTypes.def

clang/include/clang/Sema/Sema.h

clang/include/clang/Serialization/ASTBitCodes.h

clang/include/clang/module.modulemap

clang/lib/AST/ASTContext.cpp

clang/lib/AST/ASTImporter.cpp

clang/lib/AST/ExprConstant.cpp

clang/lib/AST/ItaniumMangle.cpp

clang/lib/AST/MicrosoftMangle.cpp

clang/lib/AST/NSAPI.cpp

clang/lib/AST/PrintfFormatString.cpp

clang/lib/AST/Type.cpp

clang/lib/AST/TypeLoc.cpp

clang/lib/CodeGen/CGBuiltin.cpp

clang/lib/CodeGen/CGDebugInfo.h

clang/lib/CodeGen/CGDebugInfo.cpp

clang/lib/CodeGen/CodeGenTypes.cpp

clang/lib/CodeGen/ItaniumCXXABI.cpp

clang/lib/CodeGen/TargetInfo.h

clang/lib/CodeGen/TargetInfo.cpp

clang/lib/Index/USRGeneration.cpp

clang/lib/Sema/Sema.cpp

clang/lib/Sema/SemaChecking.cpp

clang/lib/Sema/SemaDecl.cpp

clang/lib/Sema/SemaExpr.cpp

clang/lib/Sema/SemaType.cpp

clang/lib/Serialization/ASTCommon.cpp

clang/lib/Serialization/ASTReader.cpp

clang/test/CodeGen/WebAssembly/wasm-externref.c

clang/test/CodeGen/builtins-wasm.c

clang/test/CodeGenCXX/wasm-reftypes-mangle.cpp

clang/test/CodeGenCXX/wasm-reftypes-typeinfo.cpp

clang/test/Sema/wasm-refs.c

clang/test/SemaCXX/wasm-refs.cpp

clang/test/SemaTemplate/address_space-dependent.cpp

clang/tools/libclang/CIndex.cpp

llvm/include/llvm/IR/Type.h

llvm/include/llvm/Transforms/Utils.h

llvm/lib/CodeGen/ValueTypes.cpp

llvm/lib/IR/Type.cpp

llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp

llvm/lib/Transforms/Utils/Mem2Reg.cpp

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

Revision Contents
Changeset List