Page MenuHomePhabricator

[WebAssembly] Initial support for reference type funcref in clang
Changes PlannedPublic

Authored by pmatos on Jun 23 2022, 6:49 AM.

Details

Summary

This is the funcref counterpart to 104bad5a. We introduce a new attribute
that marks a function pointer as a funcref. It also implements builtin
__builtin_wasm_ref_null_func(), that returns a null funcref value.

Diff Detail

Event Timeline

pmatos created this revision.Jun 23 2022, 6:49 AM
pmatos requested review of this revision.Jun 23 2022, 6:49 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 23 2022, 6:49 AM
pmatos planned changes to this revision.Jun 23 2022, 6:49 AM

Draft patch as some tests still fail.

pmatos updated this revision to Diff 439383.Jun 23 2022, 7:10 AM

Remove conflict marker

pmatos updated this revision to Diff 445410.Jul 18 2022, 12:56 AM

Completely refactored the initial solution to funcref implementation and design.

Now it works similarly to the __ptr32 attribute from -fms-extensions.
We keep _both_ attribute and address space through the frontend, which is then automatically
converted to the proper LLVM address space through the AddressSpaceMap.

pmatos updated this revision to Diff 445524.Jul 18 2022, 8:54 AM

Cleanedup the code. Ready to review.

pmatos updated this revision to Diff 446406.Jul 21 2022, 2:58 AM

Rebase on top of main. Add missing code in Sema* to ensure the new funcref builtin is checked for.
Thanks to @asb for helping fix this.

pmatos updated this revision to Diff 446726.Jul 21 2022, 11:53 PM

Update the patch as there were some missing calls from SemaChecking.
Thanks to @asb for spotting it.

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?

Oops, sorry about that. Will take a look.

It would be good to add tests for more error conditions like in the externref patch and also additional errors like trying to apply __funcref to types that aren't function pointers.

clang/include/clang/Basic/Attr.td
4053

It would be good to document this!

clang/lib/Sema/SemaChecking.cpp
4528

Are the extra parentheses meaningful here?

6544–6547

How do these parts correspond to the original source code?

aaron.ballman requested changes to this revision.Aug 17 2022, 7:15 AM

Same question here about an RFC for adding this new type to the type system and a design document for how the type behaves as in https://reviews.llvm.org/D122215.

clang/include/clang/Basic/Attr.td
4053

It's a requirement that this be documented. Also, this should probably have a Subjects line that expects a function of some sort? (It doesn't impact the semantics of anything, but it self-documents the expectations for people reading Attr.td.)

clang/include/clang/Basic/DiagnosticSemaKinds.td
7332
  1. What is __clang_webassembly_funcref? Did you mean __funcref?
  2. There's no test coverage for this diagnostic.
clang/include/clang/Basic/TokenKinds.def
666

Why is this a keyword in all language modes?

clang/include/clang/Parse/Parser.h
2843
clang/include/clang/Sema/Sema.h
13103–13104

Formatting is off here.

13167

I think there's a verb missing from this function identifier -- also, I don't think we need to continue the terrible trend of prepending Sema onto the function name which lives in the Sema object.

clang/lib/AST/ASTContext.cpp
954

Where did this value come from?

clang/lib/Parse/ParseDecl.cpp
845–846
851–854
5780

Why that specific ordering as opposed to AR_VendorAttributesParsed?

clang/lib/Sema/SemaChecking.cpp
6549–6553

Why would we need to do this?? And what do we do when there is no FunctionDecl to get back to (because it's a call expression through a function pointer, for example)?

6556

Why are you guaranteed this function returns void?

clang/lib/Sema/SemaType.cpp
7240
7253

This seems like it's not used.

7273–7274

What happens when the user's function already has an explicitly specified address space?

This revision now requires changes to proceed.Aug 17 2022, 7:15 AM
pmatos added inline comments.Thu, Sep 15, 5:32 AM
clang/lib/AST/ASTContext.cpp
954

Unsure what you mean here. This is the address space number we also use on the LLVM side.

aaron.ballman added inline comments.Thu, Sep 15, 9:26 AM
clang/lib/AST/ASTContext.cpp
954

Unsure what you mean here. This is the address space number we also use on the LLVM side.

That's exactly what I was looking for -- I wanted to make sure this agreed with the LLVM side of things and any public documentation about address spaces for WASM.

pmatos marked 2 inline comments as done.Sun, Sep 18, 11:57 PM
pmatos added inline comments.
clang/lib/AST/ASTContext.cpp
954

Unsure what you mean here. This is the address space number we also use on the LLVM side.

That's exactly what I was looking for -- I wanted to make sure this agreed with the LLVM side of things and any public documentation about address spaces for WASM.

Sure! Marking as done.

pmatos marked 3 inline comments as done.Mon, Sep 19, 12:52 AM
pmatos added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
7332
  1. What is __clang_webassembly_funcref? Did you mean __funcref?
  2. There's no test coverage for this diagnostic.

Yes, I meant __funcref. Thanks, will add test coverage for the diagnostic.

clang/include/clang/Basic/TokenKinds.def
666

I might have misread the docs but this keyword should be available for both C and C++. Maybe I want KEYC99 | KEYCXX ?

clang/include/clang/Sema/Sema.h
13103–13104

Don't understand why. It's the same as CheckRISCVBuiltinFunctionCall ?

pmatos updated this revision to Diff 461159.Mon, Sep 19, 1:49 AM

Addressed some of the concerns from the comments but not ready for review yet.

pmatos planned changes to this revision.Mon, Sep 19, 1:49 AM
pmatos updated this revision to Diff 461544.Tue, Sep 20, 5:44 AM

Address most of the comments.

pmatos planned changes to this revision.Tue, Sep 20, 5:49 AM