[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 abandoned this revision.
sbc100 added a comment.

In D76547#4231604 , @aaron.ballman 
wrote:

> In D76547#4231501 , @sbc100 wrote:
>
>> In D76547#4231476 , @aaron.ballman 
>> wrote:
>>
>>> In D76547#4231422 , @sbc100 wrote:
>>>
 The reason `__attribute__((export_name("foo")))` doesn't work in all use 
 cases is that we have a lot of existing code that uses the 
 `EMSCRIPTEN_KEEPALIVE` macro. We also have run into other folks who 
 want to include this is some kind of `FOO_API`, or `EXPORT_API` type 
 macros.  Its not possible to have such a macro map to the existing 
 export_name since they don't include the symbol name: e.g:

   EMSCRIPTEN_KEEPALIVE int foo();`
   
   JNI_EXPORT int myfunct();

 In these cases we need something that uses the llvm symbol name for the 
 export.
>>>
>>> I think there's two ways we could address this without adding a new 
>>> attribute (maybe you've thought of this and have reasons for this to be a 
>>> bad suggestion):
>>>
>>> - It seems that `export_name` doesn't care if you put in an empty string 
>>> for the argument, so we could treat that case as meaning "export with the 
>>> name of the symbol this attribute is attached to"
>>> - We could allow `export_name` to take zero or one argument. The 
>>> one-argument form is the same as it is today, but the zero argument form 
>>> exports with the name of the symbol the attribute is attached to.
>>>
>>> Do you think either of those could work?
>>
>> Yes, I think the second one would be ideal.  The first one is slightly less 
>> idea because it prevents something being exported with the empty string as 
>> its name (wasm allows such things).
>
> Ah, yeah, that's a good reason to avoid the first one.
>
>> Can an attribute take an optional argument?  That would be great solution.   
>> For my initial version of this change I did look into making 
>> `export_name(DEFAULT)` work (note the lack of quotes around the word DEFAULT 
>> here), but I could not find way to make a single attribute take both a 
>> string *or* a constant.
>
> Yup, it takes only a little bit of extra work to do right. Have the 
> attribute's argument list take a `VariadicStringArgument` instead of 
> `StringArgument` so you can pass zero or more arguments, then have the 
> attribute handler in SemaDeclAttr.cpp diagnose if the attribute is given > 1 
> argument. The rest should be things like documentation or fall out somewhat 
> naturally (there will be accessors added to the `WebAssemblyExportNameAttr` 
> class that let you access the arguments with iterators, and a size accessor 
> as well, so you can tell if the semantic attribute does/does not have an 
> argument).

Thanks!  Closing this one for now then.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D76547#4231501 , @sbc100 wrote:

> In D76547#4231476 , @aaron.ballman 
> wrote:
>
>> In D76547#4231422 , @sbc100 wrote:
>>
>>> The reason `__attribute__((export_name("foo")))` doesn't work in all use 
>>> cases is that we have a lot of existing code that uses the 
>>> `EMSCRIPTEN_KEEPALIVE` macro. We also have run into other folks who 
>>> want to include this is some kind of `FOO_API`, or `EXPORT_API` type 
>>> macros.  Its not possible to have such a macro map to the existing 
>>> export_name since they don't include the symbol name: e.g:
>>>
>>>   EMSCRIPTEN_KEEPALIVE int foo();`
>>>   
>>>   JNI_EXPORT int myfunct();
>>>
>>> In these cases we need something that uses the llvm symbol name for the 
>>> export.
>>
>> I think there's two ways we could address this without adding a new 
>> attribute (maybe you've thought of this and have reasons for this to be a 
>> bad suggestion):
>>
>> - It seems that `export_name` doesn't care if you put in an empty string for 
>> the argument, so we could treat that case as meaning "export with the name 
>> of the symbol this attribute is attached to"
>> - We could allow `export_name` to take zero or one argument. The 
>> one-argument form is the same as it is today, but the zero argument form 
>> exports with the name of the symbol the attribute is attached to.
>>
>> Do you think either of those could work?
>
> Yes, I think the second one would be ideal.  The first one is slightly less 
> idea because it prevents something being exported with the empty string as 
> its name (wasm allows such things).

Ah, yeah, that's a good reason to avoid the first one.

> Can an attribute take an optional argument?  That would be great solution.   
> For my initial version of this change I did look into making 
> `export_name(DEFAULT)` work (note the lack of quotes around the word DEFAULT 
> here), but I could not find way to make a single attribute take both a string 
> *or* a constant.

Yup, it takes only a little bit of extra work to do right. Have the attribute's 
argument list take a `VariadicStringArgument` instead of `StringArgument` so 
you can pass zero or more arguments, then have the attribute handler in 
SemaDeclAttr.cpp diagnose if the attribute is given > 1 argument. The rest 
should be things like documentation or fall out somewhat naturally (there will 
be accessors added to the `WebAssemblyExportNameAttr` class that let you access 
the arguments with iterators, and a size accessor as well, so you can tell if 
the semantic attribute does/does not have an argument).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

In D76547#4231476 , @aaron.ballman 
wrote:

> In D76547#4231422 , @sbc100 wrote:
>
>> The reason `__attribute__((export_name("foo")))` doesn't work in all use 
>> cases is that we have a lot of existing code that uses the 
>> `EMSCRIPTEN_KEEPALIVE` macro. We also have run into other folks who want 
>> to include this is some kind of `FOO_API`, or `EXPORT_API` type macros.  Its 
>> not possible to have such a macro map to the existing export_name since they 
>> don't include the symbol name: e.g:
>>
>>   EMSCRIPTEN_KEEPALIVE int foo();`
>>   
>>   JNI_EXPORT int myfunct();
>>
>> In these cases we need something that uses the llvm symbol name for the 
>> export.
>
> I think there's two ways we could address this without adding a new attribute 
> (maybe you've thought of this and have reasons for this to be a bad 
> suggestion):
>
> - It seems that `export_name` doesn't care if you put in an empty string for 
> the argument, so we could treat that case as meaning "export with the name of 
> the symbol this attribute is attached to"
> - We could allow `export_name` to take zero or one argument. The one-argument 
> form is the same as it is today, but the zero argument form exports with the 
> name of the symbol the attribute is attached to.
>
> Do you think either of those could work?

Yes, I think the second one would be ideal.  The first one is slightly less 
idea because it prevents something being exported with the empty string as its 
name (wasm allows such things).

Can an attribute take an optional argument?  That would be great solution.   
For my initial version of this change I did look into making 
`export_name(DEFAULT)` work (note the lack of quotes around the word DEFAULT 
here), but I could not find way to make a single attribute take both a string 
*or* a constant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D76547#4231422 , @sbc100 wrote:

> The reason `__attribute__((export_name("foo")))` doesn't work in all use 
> cases is that we have a lot of existing code that uses the 
> `EMSCRIPTEN_KEEPALIVE` macro. We also have run into other folks who want 
> to include this is some kind of `FOO_API`, or `EXPORT_API` type macros.  Its 
> not possible to have such a macro map to the existing export_name since they 
> don't include the symbol name: e.g:
>
>   EMSCRIPTEN_KEEPALIVE int foo();`
>   
>   JNI_EXPORT int myfunct();
>
> In these cases we need something that uses the llvm symbol name for the 
> export.

I think there's two ways we could address this without adding a new attribute 
(maybe you've thought of this and have reasons for this to be a bad suggestion):

- It seems that `export_name` doesn't care if you put in an empty string for 
the argument, so we could treat that case as meaning "export with the name of 
the symbol this attribute is attached to"
- We could allow `export_name` to take zero or one argument. The one-argument 
form is the same as it is today, but the zero argument form exports with the 
name of the symbol the attribute is attached to.

Do you think either of those could work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

The reason `__attribute__((export_name("foo")))` doesn't work in all use cases 
is that we have a lot of existing code that uses the `EMSCRIPTEN_KEEPALIVE` 
macro. We also have run into other folks who want to include this is some 
kind of `FOO_API`, or `EXPORT_API` type macros.  Its not possible to have such 
a macro map to the existing export_name since they don't include the symbol 
name: e.g:

  EMSCRIPTEN_KEEPALIVE int foo();`
  
  JNI_EXPORT int myfunct();

In these cases we need something that uses the llvm symbol name for the export.

However, as I mention in my earlier comment I have since realized that these 
attributes cannot be applies the llvm GlobalValues in general only the 
Functions... and the idea behind EMSCRIPTEN_KEEPALIVE is that it can be used to 
tag both functions and data symbols (addresses).

Sadly I don't see any way to attach attributes to data symbols in llvm today..  
we have visibility but not attribute bag :(




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7572
+  if (FD->isThisDeclarationADefinition()) {
+S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
+return;

aaron.ballman wrote:
> Is this diagnostic actually correct? It's for use with the alias and ifunc 
> attributes, so I'm surprised to see it here.
It does look incorrect yes, this is copy-pasted from the exist `ExportNameAttr` 
so I guess that is wrong too. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

The changes need a release note at some point, and this is missing all of the 
usual sema diagnostic tests (wrong subject, wrong number of args, wrong target, 
etc).

That said, are we sure this attribute is sufficiently compelling to add it in 
the first place? This seems like an attribute needed for one? use case, but 
it's using a pretty general name for the attribute (`exported`). For example, 
it's not clear to me why this cannot be solved with the existing `export_name` 
attribute (e.g., `__attribute__((export_name("foo"))) void foo();`)




Comment at: clang/include/clang/Basic/AttrDocs.td:5533-5536
+Clang supports the ``__attribute__((exported))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function declaration, where it causes the symbol to be exported from the
+linked WebAssembly module.

I think more details would be helpful here. The docs for `export_name`:

> WebAssembly functions are exported via string name. By default when a symbol 
> is exported, the export name for C/C++ symbols are the same as their C/C++ 
> symbol names. This attribute can be used to override the default behavior, 
> and request a specific string name be used instead.

make it sound like there's already a way to export the symbol, so it's not 
clear to me why this attribute is needed (unless this attribute is actually the 
one intended to provide that functionality!).

Also, the docs don't mention important things like that this attribute only 
works for emscripten.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7561-7568
+  if (!isFunctionOrMethod(D)) {
+S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+<< "'exported'" << ExpectedFunction;
+return;
+  }
+
+  if (S.Context.getTargetInfo().getTriple().isOSEmscripten()) {

These can both be dropped, they're handled automatically for you.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7572
+  if (FD->isThisDeclarationADefinition()) {
+S.Diag(D->getLocation(), diag::err_alias_is_definition) << FD << 0;
+return;

Is this diagnostic actually correct? It's for use with the alias and ifunc 
attributes, so I'm surprised to see it here.



Comment at: clang/test/CodeGen/WebAssembly/wasm-exported.c:12
+
+// MISSING: error: unknown attribute 'exported' ignored
+

This should be handled in a sema test along with the other sema testing, rather 
than squirreled away here in the codegen tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

I just figured out that this cannot replace the current use of 
`__attribute__((used))` in emscripten because function attributes only work for 
functions and we need this mechanism to work for global data addresses too. 
There is simply no way to do something like `Fn->addFnAttr("wasm-exported");` 
for a GlobalValue that isn't a function (as far as I can tell).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 updated this revision to Diff 507826.
sbc100 edited the summary of this revision.
sbc100 added a comment.

- limit to emscripten


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/WebAssembly/wasm-exported.c
  llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/test/CodeGen/WebAssembly/export.ll

Index: llvm/test/CodeGen/WebAssembly/export.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/export.ll
@@ -0,0 +1,16 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-emscripten"
+
+define void @test() #0 {
+  ret void
+}
+
+declare void @test2() #1
+
+attributes #0 = { "wasm-exported" }
+attributes #1 = { "wasm-exported" }
+
+; CHECK: .export test
+; CHECK: .export test2
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -376,6 +376,12 @@
   getTargetStreamer()->emitImportName(Sym, Name);
 }
 
+if (F.hasFnAttribute("wasm-exported")) {
+  auto *Sym = cast(getSymbol(&F));
+  Sym->setExported();
+  getTargetStreamer()->emitExport(Sym);
+}
+
 if (F.hasFnAttribute("wasm-export-name")) {
   auto *Sym = cast(getSymbol(&F));
   StringRef Name = F.getFnAttribute("wasm-export-name").getValueAsString();
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
@@ -51,6 +51,8 @@
   /// .export_name
   virtual void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) = 0;
+  /// .export
+  virtual void emitExport(const MCSymbolWasm *Sym) = 0;
 
 protected:
   void emitValueType(wasm::ValType Type);
@@ -72,6 +74,7 @@
   void emitImportModule(const MCSymbolWasm *Sym, StringRef ImportModule) override;
   void emitImportName(const MCSymbolWasm *Sym, StringRef ImportName) override;
   void emitExportName(const MCSymbolWasm *Sym, StringRef ExportName) override;
+  void emitExport(const MCSymbolWasm *Sym) override;
 };
 
 /// This part is for Wasm object output
@@ -91,6 +94,7 @@
   StringRef ImportName) override {}
   void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) override {}
+  void emitExport(const MCSymbolWasm *Sym) override {}
 };
 
 /// This part is for null output
@@ -108,6 +112,7 @@
   void emitImportModule(const MCSymbolWasm *, StringRef) override {}
   void emitImportName(const MCSymbolWasm *, StringRef) override {}
   void emitExportName(const MCSymbolWasm *, StringRef) override {}
+  void emitExport(const MCSymbolWasm *) override {}
 };
 
 } // end namespace llvm
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
@@ -114,6 +114,10 @@
<< ExportName << '\n';
 }
 
+void WebAssemblyTargetAsmStreamer::emitExport(const MCSymbolWasm *Sym) {
+  OS << "\t.export\t" << Sym->getName() << '\n';
+}
+
 void WebAssemblyTargetAsmStreamer::emitIndIdx(const MCExpr *Value) {
   OS << "\t.indidx  \t" << *Value << '\n';
 }
Index: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
===
--- llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
+++ llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
@@ -897,6 +897,15 @@
   return expect(AsmToken::EndOfStatement, "EOL");
 }
 
+if (DirectiveID.getString() == ".export") {
+  auto SymName = expectIdent();
+  if (SymName.empty())
+return true;
+  auto WasmSym = cast(Ctx.getOrCreateSymbol(SymName));
+  WasmSym->setExported();
+  TOut.emitExport(WasmSym);
+}
+
 if (DirectiveID.getString() == ".export_name") {
   auto SymName = expectIdent();
   if (SymName.empty())
Index: clang/test/CodeGen/WebAssembly/wasm-exported.c

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

I've limited to new attribute to only the emcripten triple.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add `wasm-exported` function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 updated this revision to Diff 507807.
sbc100 added a comment.

- update test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/wasm-export.c
  llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/test/CodeGen/WebAssembly/export.ll

Index: llvm/test/CodeGen/WebAssembly/export.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/export.ll
@@ -0,0 +1,17 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+define void @test() #0 {
+  ret void
+}
+
+declare void @test2() #1
+
+
+attributes #0 = { "wasm-exported" }
+attributes #1 = { "wasm-exported" }
+
+; CHECK: .export test
+; CHECK: .export test2
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -376,6 +376,12 @@
   getTargetStreamer()->emitImportName(Sym, Name);
 }
 
+if (F.hasFnAttribute("wasm-exported")) {
+  auto *Sym = cast(getSymbol(&F));
+  Sym->setExported();
+  getTargetStreamer()->emitExport(Sym);
+}
+
 if (F.hasFnAttribute("wasm-export-name")) {
   auto *Sym = cast(getSymbol(&F));
   StringRef Name = F.getFnAttribute("wasm-export-name").getValueAsString();
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
@@ -51,6 +51,8 @@
   /// .export_name
   virtual void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) = 0;
+  /// .export
+  virtual void emitExport(const MCSymbolWasm *Sym) = 0;
 
 protected:
   void emitValueType(wasm::ValType Type);
@@ -72,6 +74,7 @@
   void emitImportModule(const MCSymbolWasm *Sym, StringRef ImportModule) override;
   void emitImportName(const MCSymbolWasm *Sym, StringRef ImportName) override;
   void emitExportName(const MCSymbolWasm *Sym, StringRef ExportName) override;
+  void emitExport(const MCSymbolWasm *Sym) override;
 };
 
 /// This part is for Wasm object output
@@ -91,6 +94,7 @@
   StringRef ImportName) override {}
   void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) override {}
+  void emitExport(const MCSymbolWasm *Sym) override {}
 };
 
 /// This part is for null output
@@ -108,6 +112,7 @@
   void emitImportModule(const MCSymbolWasm *, StringRef) override {}
   void emitImportName(const MCSymbolWasm *, StringRef) override {}
   void emitExportName(const MCSymbolWasm *, StringRef) override {}
+  void emitExport(const MCSymbolWasm *) override {}
 };
 
 } // end namespace llvm
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
@@ -114,6 +114,10 @@
<< ExportName << '\n';
 }
 
+void WebAssemblyTargetAsmStreamer::emitExport(const MCSymbolWasm *Sym) {
+  OS << "\t.export\t" << Sym->getName() << '\n';
+}
+
 void WebAssemblyTargetAsmStreamer::emitIndIdx(const MCExpr *Value) {
   OS << "\t.indidx  \t" << *Value << '\n';
 }
Index: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
===
--- llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
+++ llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
@@ -897,6 +897,15 @@
   return expect(AsmToken::EndOfStatement, "EOL");
 }
 
+if (DirectiveID.getString() == ".export") {
+  auto SymName = expectIdent();
+  if (SymName.empty())
+return true;
+  auto WasmSym = cast(Ctx.getOrCreateSymbol(SymName));
+  WasmSym->setExported();
+  TOut.emitExport(WasmSym);
+}
+
 if (DirectiveID.getString() == ".export_name") {
   auto SymName = expectIdent();
   if (SymName.empty())
Index: clang/test/CodeGen/wasm-export.c
===
--- /dev/null
+++ clang/test/CodeGen/wasm-export.c
@@ -0,0 +1

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

In D76547#1945094 , @sbc100 wrote:

> What about your idea of using the `default` keyword rather than adding a new 
> clang attr?  I quite liked that approach.

IIRC I tried this approach but wan't able to make it works since a single 
attribute can't both take a string and a fix value like `default`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 updated this revision to Diff 507802.
sbc100 added a comment.
Herald added a reviewer: aaron.ballman.
Herald added subscribers: llvm-commits, pmatos, asb, wingo, ecnelises.
Herald added projects: LLVM, All.

- rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/wasm-export.c
  llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/test/CodeGen/WebAssembly/export.ll

Index: llvm/test/CodeGen/WebAssembly/export.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/export.ll
@@ -0,0 +1,17 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+define void @test() #0 {
+  ret void
+}
+
+declare void @test2() #1
+
+
+attributes #0 = { "wasm-exported" }
+attributes #1 = { "wasm-exported" }
+
+; CHECK: .export test
+; CHECK: .export test2
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -376,6 +376,12 @@
   getTargetStreamer()->emitImportName(Sym, Name);
 }
 
+if (F.hasFnAttribute("wasm-exported")) {
+  auto *Sym = cast(getSymbol(&F));
+  Sym->setExported();
+  getTargetStreamer()->emitExport(Sym);
+}
+
 if (F.hasFnAttribute("wasm-export-name")) {
   auto *Sym = cast(getSymbol(&F));
   StringRef Name = F.getFnAttribute("wasm-export-name").getValueAsString();
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
@@ -51,6 +51,8 @@
   /// .export_name
   virtual void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) = 0;
+  /// .export
+  virtual void emitExport(const MCSymbolWasm *Sym) = 0;
 
 protected:
   void emitValueType(wasm::ValType Type);
@@ -72,6 +74,7 @@
   void emitImportModule(const MCSymbolWasm *Sym, StringRef ImportModule) override;
   void emitImportName(const MCSymbolWasm *Sym, StringRef ImportName) override;
   void emitExportName(const MCSymbolWasm *Sym, StringRef ExportName) override;
+  void emitExport(const MCSymbolWasm *Sym) override;
 };
 
 /// This part is for Wasm object output
@@ -91,6 +94,7 @@
   StringRef ImportName) override {}
   void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) override {}
+  void emitExport(const MCSymbolWasm *Sym) override {}
 };
 
 /// This part is for null output
@@ -108,6 +112,7 @@
   void emitImportModule(const MCSymbolWasm *, StringRef) override {}
   void emitImportName(const MCSymbolWasm *, StringRef) override {}
   void emitExportName(const MCSymbolWasm *, StringRef) override {}
+  void emitExport(const MCSymbolWasm *) override {}
 };
 
 } // end namespace llvm
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
@@ -114,6 +114,10 @@
<< ExportName << '\n';
 }
 
+void WebAssemblyTargetAsmStreamer::emitExport(const MCSymbolWasm *Sym) {
+  OS << "\t.export\t" << Sym->getName() << '\n';
+}
+
 void WebAssemblyTargetAsmStreamer::emitIndIdx(const MCExpr *Value) {
   OS << "\t.indidx  \t" << *Value << '\n';
 }
Index: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
===
--- llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
+++ llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
@@ -897,6 +897,15 @@
   return expect(AsmToken::EndOfStatement, "EOL");
 }
 
+if (DirectiveID.getString() == ".export") {
+  auto SymName = expectIdent();
+  if (SymName.empty())
+return true;
+  auto WasmSym = cast(Ctx.getOrCreateSymbol(SymName));
+  WasmSym->setExported();
+  TOut.emitExport(WasmSym);
+}
+
 if (DirectiveID.getString() == ".export_name") {
   auto SymName = expectIdent();
   if (SymName.empty())
Index: clang/test/CodeGen/was

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

I was thinking of dropping the new clang attr in favor of the `default` keyword 
in the current attr, but actually keeping the llvm attr, since it corresponds 
quite nicely to the existing EXPORTED symbol attribute in the binary format and 
avoid duplication in the `.ll`, `.s` and `.o` formats.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

What about your idea of using the `default` keyword rather than adding a new 
clang attr?  I quite liked that approach.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Dan Gohman via Phabricator via cfe-commits]

sunfish added a comment.

Instead of creating a new LLVM-IR-level attribute here, could you have clang 
translate the attribute to "wasm-export-name", to keep the LLVM-IR level 
simpler?

Also, I myself would be more comfortable with this change if it were restricted 
to Emscripten for now. `export_name` already exists and works in both 
Emscripten and non-Emscripten targets. If there's demand for this new syntax 
outside of Emscripten, I'm happy to reconsider, but until then it seems better 
to be conservative. Obviously it's not possible to completely prevent people 
from becoming dependent on C++ ABI details, but we can avoid giving them tools 
that make it easy to do the wrong thing. And we can keep the ecosystem simpler 
if we don't have multiple ways to do the same thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Alon Zakai via Phabricator via cfe-commits]

kripken accepted this revision.
kripken added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Basic/AttrDocs.td:4173
+attribute for the WebAssembly target. This attribute may be attached to a
+function declaration, where it causes the be exported from the linked
+WebAssembly module.

"the be" => "the function to be"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76547/new/

https://reviews.llvm.org/D76547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

[Sam Clegg via Phabricator via cfe-commits]

sbc100 created this revision.
Herald added subscribers: cfe-commits, sunfish, aheejin, hiraditya, 
jgravelle-google, dschuff.
Herald added a project: clang.
sbc100 added reviewers: sunfish, kripken.

This matches the existing export-name attribute but exports that symbol
but its llvm symbol name.  This corresponds directly to the existing
WASM_SYMBOL_EXPORTED symbol flag.

This allows the existing emscripgten macro `EMSCERIPTEN_KEEPALIVE` to be
implemented in terms of this attribute rather then the current
workaround which uses the `used` attribute.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76547

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/TargetInfo.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/wasm-export.c
  llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/test/CodeGen/WebAssembly/export.ll

Index: llvm/test/CodeGen/WebAssembly/export.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/export.ll
@@ -0,0 +1,17 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+define void @test() #0 {
+  ret void
+}
+
+declare void @test2() #1
+
+
+attributes #0 = { "wasm-exported" }
+attributes #1 = { "wasm-exported" }
+
+; CHECK: .export test
+; CHECK: .export test2
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -134,6 +134,12 @@
   }
 }
 
+if (F.hasFnAttribute("wasm-exported")) {
+  auto *Sym = cast(getSymbol(&F));
+  Sym->setExported();
+  getTargetStreamer()->emitExport(Sym);
+}
+
 if (F.hasFnAttribute("wasm-export-name")) {
   auto *Sym = cast(getSymbol(&F));
   StringRef Name = F.getFnAttribute("wasm-export-name").getValueAsString();
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h
@@ -51,6 +51,8 @@
   /// .export_name
   virtual void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) = 0;
+  /// .export
+  virtual void emitExport(const MCSymbolWasm *Sym) = 0;
 
 protected:
   void emitValueType(wasm::ValType Type);
@@ -72,6 +74,7 @@
   void emitImportModule(const MCSymbolWasm *Sym, StringRef ImportModule) override;
   void emitImportName(const MCSymbolWasm *Sym, StringRef ImportName) override;
   void emitExportName(const MCSymbolWasm *Sym, StringRef ExportName) override;
+  void emitExport(const MCSymbolWasm *Sym) override;
 };
 
 /// This part is for Wasm object output
@@ -91,6 +94,7 @@
   StringRef ImportName) override {}
   void emitExportName(const MCSymbolWasm *Sym,
   StringRef ExportName) override {}
+  void emitExport(const MCSymbolWasm *Sym) override {}
 };
 
 /// This part is for null output
@@ -108,6 +112,7 @@
   void emitImportModule(const MCSymbolWasm *, StringRef) override {}
   void emitImportName(const MCSymbolWasm *, StringRef) override {}
   void emitExportName(const MCSymbolWasm *, StringRef) override {}
+  void emitExport(const MCSymbolWasm *) override {}
 };
 
 } // end namespace llvm
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
===
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
@@ -100,6 +100,10 @@
<< ExportName << '\n';
 }
 
+void WebAssemblyTargetAsmStreamer::emitExport(const MCSymbolWasm *Sym) {
+  OS << "\t.export\t" << Sym->getName() << '\n';
+}
+
 void WebAssemblyTargetAsmStreamer::emitIndIdx(const MCExpr *Value) {
   OS << "\t.indidx  \t" << *Value << '\n';
 }
Index: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
===
--- llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
+++ llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
@@ -717,6 +717,15 @@
   return expect(AsmToken::EndOfStatement, "EOL");
 }
 
+if (DirectiveID.getString() == ".export") {
+  auto SymName = expectIdent();
+  if (SymName.empty())
+return true;
+  auto WasmSym = cast(Ctx.getOrCreateSymbol(SymName));
+