[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-27 Thread Sam Clegg via Phabricator via cfe-commits 
sbc100 added a comment.

In D150803#4454164 , @brendandahl 
wrote:

> In D150803#4440802 , @aaron.ballman 
> wrote:
>
>> Marking as requested changes so it's clear there's more worth discussing, so 
>> we don't accidentally land this.
>
> I've switched to using `annotate` now. Let me know if there's anything else.

I guess the CL title and description now need updating?   Nice to see we no 
longer need any clang changes here!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-27 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl added a comment.

In D150803#4440802 , @aaron.ballman 
wrote:

> Marking as requested changes so it's clear there's more worth discussing, so 
> we don't accidentally land this.

I've switched to using `annotate` now. Let me know if there's anything else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-27 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl updated this revision to Diff 535126.
brendandahl added a comment.

Use the annotate attribute to generate custom sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

Files:
  lld/test/wasm/func-attr-tombstone.s
  lld/test/wasm/func-attr.s
  lld/test/wasm/merge-func-attr-section.s
  lld/wasm/InputChunks.cpp
  lld/wasm/InputFiles.cpp
  llvm/docs/ReleaseNotes.rst
  llvm/include/llvm/BinaryFormat/WasmRelocs.def
  llvm/include/llvm/MC/MCExpr.h
  llvm/lib/MC/MCExpr.cpp
  llvm/lib/MC/WasmObjectWriter.cpp
  llvm/lib/Object/WasmObjectFile.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
  llvm/test/CodeGen/WebAssembly/func-attr-annotate.ll
  llvm/test/MC/WebAssembly/func-attr.s

Index: llvm/test/MC/WebAssembly/func-attr.s
===
--- /dev/null
+++ llvm/test/MC/WebAssembly/func-attr.s
@@ -0,0 +1,21 @@
+# RUN: llvm-mc -triple=wasm32-unknown-unknown < %s | FileCheck %s
+# Check that it also comiled to object for format.
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -o - < %s | obj2yaml | FileCheck -check-prefix=CHECK-OBJ %s
+
+foo:
+.globl foo
+.functype foo () -> ()
+end_function
+
+.section.custom_section.func_attr.custom0,"",@
+.int32  foo@FUNCINDEX
+
+# CHECK:   .section .custom_section.func_attr.custom0,"",@
+# CHECK-NEXT: .int32  foo@FUNCINDEX
+
+# CHECK-OBJ:- Type:CUSTOM
+# CHECK-OBJ-NEXT: Relocations:
+# CHECK-OBJ-NEXT:- Type:R_WASM_FUNCTION_INDEX_I32
+# CHECK-OBJ-NEXT:  Index:   0
+# CHECK-OBJ-NEXT:  Offset:  0x0
+# CHECK-OBJ-NEXT: Name:func_attr.custom0
Index: llvm/test/CodeGen/WebAssembly/func-attr-annotate.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/func-attr-annotate.ll
@@ -0,0 +1,36 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+@.str = private unnamed_addr constant [8 x i8] c"custom0\00", section "llvm.metadata"
+@.str.1 = private unnamed_addr constant [7 x i8] c"main.c\00", section "llvm.metadata"
+@.str.2 = private unnamed_addr constant [8 x i8] c"custom1\00", section "llvm.metadata"
+@.str.3 = private unnamed_addr constant [8 x i8] c"custom2\00", section "llvm.metadata"
+@.str.4 = private unnamed_addr constant [12 x i8] c"custom2arg0\00", align 1
+@.str.5 = private unnamed_addr constant [12 x i8] c"custom2arg1\00", align 1
+@.args = private unnamed_addr constant { ptr, ptr } { ptr @.str.4, ptr @.str.5 }, section "llvm.metadata"
+@llvm.global.annotations = appending global [4 x { ptr, ptr, ptr, i32, ptr }] [{ ptr, ptr, ptr, i32, ptr } { ptr @test0, ptr @.str, ptr @.str.1, i32 4, ptr null }, { ptr, ptr, ptr, i32, ptr } { ptr @test1, ptr @.str, ptr @.str.1, i32 5, ptr null }, { ptr, ptr, ptr, i32, ptr } { ptr @test2, ptr @.str.2, ptr @.str.1, i32 6, ptr null }, { ptr, ptr, ptr, i32, ptr } { ptr @test3, ptr @.str.3, ptr @.str.1, i32 7, ptr @.args }], section "llvm.metadata"
+
+define void @test0() {
+  ret void
+}
+
+define void @test1() {
+  ret void
+}
+
+define void @test2() {
+  ret void
+}
+
+define void @test3() {
+  ret void
+}
+
+; CHECK:  .section.custom_section.func_attr.annotate.custom2.custom2arg0.custom2arg1,"",@
+; CHECK-NEXT: .int32  test3@FUNCINDEX
+; CHECK:  .section.custom_section.func_attr.annotate.custom0,"",@
+; CHECK-NEXT: .int32  test0@FUNCINDEX
+; CHECK-NEXT: .int32  test1@FUNCINDEX
+; CHECK:  .section.custom_section.func_attr.annotate.custom1,"",@
+; CHECK-NEXT: .int32  test2@FUNCINDEX
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
@@ -66,6 +66,7 @@
   void emitEndOfAsmFile(Module ) override;
   void EmitProducerInfo(Module );
   void EmitTargetFeatures(Module );
+  void EmitFunctionAttributes(Module );
   void emitSymbolType(const MCSymbolWasm *Sym);
   void emitGlobalVariable(const GlobalVariable *GV) override;
   void emitJumpTableInfo() override;
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -27,6 +27,8 @@
 #include "WebAssemblyTargetMachine.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/CodeGen/Analysis.h"

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-22 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

sbc100 wrote:
> sbc100 wrote:
> > sbc100 wrote:
> > > aaron.ballman wrote:
> > > > brendandahl wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > This could be my ignorance of web assembly showing, but the docs 
> > > > > > > don't really help me understand when you'd want to use this 
> > > > > > > attribute. Perhaps a link to what JSPI is and a code example 
> > > > > > > would help a little bit? Or is this more of a low-level 
> > > > > > > implementation detail kind of attribute where folks already know 
> > > > > > > the domain?
> > > > > > Based on the documentation here, I'm wondering why the `annotate` 
> > > > > > attribute doesn't suffice? That attribute lets you specify custom 
> > > > > > information to associate with a declaration that then gets lowered 
> > > > > > such that passes can do whatever they want with the info, which 
> > > > > > seems to be a more generalized version of what this attribute is.
> > > > > > 
> > > > > > (FWIW, I'm back to having basically no idea when you'd use this 
> > > > > > attribute or what it would be used for, so my thoughts above might 
> > > > > > make no sense.)
> > > > > I was considering that, but it would require more machinery in the 
> > > > > wasm backend to store all the attribute values in the output. For now 
> > > > > we only really need a flag associated with function. I think if we 
> > > > > find more uses for the annotations in the future we could replace 
> > > > > wasm_custom with it.
> > > > > I was considering that, but it would require more machinery in the 
> > > > > wasm backend to store all the attribute values in the output. For now 
> > > > > we only really need a flag associated with function. I think if we 
> > > > > find more uses for the annotations in the future we could replace 
> > > > > wasm_custom with it.
> > > > 
> > > > More machinery in the backend is preferred to exposing a new attribute 
> > > > that is this general-purpose; the backend is what needs this 
> > > > functionality, the frontend basically does nothing with it. (I'm 
> > > > assuming this is an implementation detail attribute and not something 
> > > > you expect users to write. If I'm wrong about that, please let me know.)
> > > > Based on the documentation here, I'm wondering why the `annotate` 
> > > > attribute doesn't suffice? 
> > > 
> > > I believe that was the original solution that was considered but Benden 
> > > was told this was perhaps not an appropriate use of "annotate".   Brenden 
> > > can you elaborate on why?   IIRC it was something like "the semantics of 
> > > the program should not depend on annotate attributes but the attribute 
> > > being added in this case is required for the program to run correctly".. 
> > > is that about right?
> > > 
> > > FWIW, I think using the existing `annotate` attribute would make a lot of 
> > > sense... if we can get away with it.
> > > (I'm assuming this is an implementation detail attribute and not 
> > > something you expect users to write. If I'm wrong about that, please let 
> > > me know.)
> > 
> > IIUC user would indeed be specifying these attributes.   Kind of like they 
> > do for "visibility" today.   The initial attribute that motivated this 
> > change is "async" which tells the host runtime that a certain function 
> > import or export behaves in an async manor (See https://v8.dev/blog/jspi 
> > for more details).
> > > I was considering that, but it would require more machinery in the wasm 
> > > backend to store all the attribute values in the output. For now we only 
> > > really need a flag associated with function. I think if we find more uses 
> > > for the annotations in the future we could replace wasm_custom with it.
> > 
> > More machinery in the backend is preferred to exposing a new attribute that 
> > is this general-purpose; the backend is what needs this functionality, the 
> > frontend basically does nothing with it. (I'm assuming this is an 
> > implementation detail attribute and not something you expect users to 
> > write. If I'm wrong about that, please let me know.)
> 
> I think perhaps there are two alternative implementations being proposed 
> here, and I want to make sure we don't confuse them:
> 
> 1. Use the existing `annotate` attribute.  
> 2. Make an even more complex custom attribute that hold key=value pairs 
> rather than the current CL which proposes simply boolean custom attributes 
> (e.g. key=true).
> 
> I think we would be happy

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-22 Thread Sam Clegg via Phabricator via cfe-commits 
sbc100 added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

sbc100 wrote:
> sbc100 wrote:
> > aaron.ballman wrote:
> > > brendandahl wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > This could be my ignorance of web assembly showing, but the docs 
> > > > > > don't really help me understand when you'd want to use this 
> > > > > > attribute. Perhaps a link to what JSPI is and a code example would 
> > > > > > help a little bit? Or is this more of a low-level implementation 
> > > > > > detail kind of attribute where folks already know the domain?
> > > > > Based on the documentation here, I'm wondering why the `annotate` 
> > > > > attribute doesn't suffice? That attribute lets you specify custom 
> > > > > information to associate with a declaration that then gets lowered 
> > > > > such that passes can do whatever they want with the info, which seems 
> > > > > to be a more generalized version of what this attribute is.
> > > > > 
> > > > > (FWIW, I'm back to having basically no idea when you'd use this 
> > > > > attribute or what it would be used for, so my thoughts above might 
> > > > > make no sense.)
> > > > I was considering that, but it would require more machinery in the wasm 
> > > > backend to store all the attribute values in the output. For now we 
> > > > only really need a flag associated with function. I think if we find 
> > > > more uses for the annotations in the future we could replace 
> > > > wasm_custom with it.
> > > > I was considering that, but it would require more machinery in the wasm 
> > > > backend to store all the attribute values in the output. For now we 
> > > > only really need a flag associated with function. I think if we find 
> > > > more uses for the annotations in the future we could replace 
> > > > wasm_custom with it.
> > > 
> > > More machinery in the backend is preferred to exposing a new attribute 
> > > that is this general-purpose; the backend is what needs this 
> > > functionality, the frontend basically does nothing with it. (I'm assuming 
> > > this is an implementation detail attribute and not something you expect 
> > > users to write. If I'm wrong about that, please let me know.)
> > > Based on the documentation here, I'm wondering why the `annotate` 
> > > attribute doesn't suffice? 
> > 
> > I believe that was the original solution that was considered but Benden was 
> > told this was perhaps not an appropriate use of "annotate".   Brenden can 
> > you elaborate on why?   IIRC it was something like "the semantics of the 
> > program should not depend on annotate attributes but the attribute being 
> > added in this case is required for the program to run correctly".. is that 
> > about right?
> > 
> > FWIW, I think using the existing `annotate` attribute would make a lot of 
> > sense... if we can get away with it.
> > (I'm assuming this is an implementation detail attribute and not something 
> > you expect users to write. If I'm wrong about that, please let me know.)
> 
> IIUC user would indeed be specifying these attributes.   Kind of like they do 
> for "visibility" today.   The initial attribute that motivated this change is 
> "async" which tells the host runtime that a certain function import or export 
> behaves in an async manor (See https://v8.dev/blog/jspi for more details).
> > I was considering that, but it would require more machinery in the wasm 
> > backend to store all the attribute values in the output. For now we only 
> > really need a flag associated with function. I think if we find more uses 
> > for the annotations in the future we could replace wasm_custom with it.
> 
> More machinery in the backend is preferred to exposing a new attribute that 
> is this general-purpose; the backend is what needs this functionality, the 
> frontend basically does nothing with it. (I'm assuming this is an 
> implementation detail attribute and not something you expect users to write. 
> If I'm wrong about that, please let me know.)

I think perhaps there are two alternative implementations being proposed here, 
and I want to make sure we don't confuse them:

1. Use the existing `annotate` attribute.  
2. Make an even more complex custom attribute that hold key=value pairs rather 
than the current CL which proposes simply boolean custom attributes (e.g. 
key=true).

I think we would be happy with (1) if this usage is within scope.

IIUC the thing that would take a lot more work in the backend (and more complex 
custom section design in the binary format) is (2).   I don't think we need

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-22 Thread Sam Clegg via Phabricator via cfe-commits 
sbc100 added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

aaron.ballman wrote:
> brendandahl wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > This could be my ignorance of web assembly showing, but the docs don't 
> > > > really help me understand when you'd want to use this attribute. 
> > > > Perhaps a link to what JSPI is and a code example would help a little 
> > > > bit? Or is this more of a low-level implementation detail kind of 
> > > > attribute where folks already know the domain?
> > > Based on the documentation here, I'm wondering why the `annotate` 
> > > attribute doesn't suffice? That attribute lets you specify custom 
> > > information to associate with a declaration that then gets lowered such 
> > > that passes can do whatever they want with the info, which seems to be a 
> > > more generalized version of what this attribute is.
> > > 
> > > (FWIW, I'm back to having basically no idea when you'd use this attribute 
> > > or what it would be used for, so my thoughts above might make no sense.)
> > I was considering that, but it would require more machinery in the wasm 
> > backend to store all the attribute values in the output. For now we only 
> > really need a flag associated with function. I think if we find more uses 
> > for the annotations in the future we could replace wasm_custom with it.
> > I was considering that, but it would require more machinery in the wasm 
> > backend to store all the attribute values in the output. For now we only 
> > really need a flag associated with function. I think if we find more uses 
> > for the annotations in the future we could replace wasm_custom with it.
> 
> More machinery in the backend is preferred to exposing a new attribute that 
> is this general-purpose; the backend is what needs this functionality, the 
> frontend basically does nothing with it. (I'm assuming this is an 
> implementation detail attribute and not something you expect users to write. 
> If I'm wrong about that, please let me know.)
> Based on the documentation here, I'm wondering why the `annotate` attribute 
> doesn't suffice? 

I believe that was the original solution that was considered but Benden was 
told this was perhaps not an appropriate use of "annotate".   Brenden can you 
elaborate on why?   IIRC it was something like "the semantics of the program 
should not depend on annotate attributes but the attribute being added in this 
case is required for the program to run correctly".. is that about right?

FWIW, I think using the existing `annotate` attribute would make a lot of 
sense... if we can get away with it.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

sbc100 wrote:
> aaron.ballman wrote:
> > brendandahl wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > This could be my ignorance of web assembly showing, but the docs 
> > > > > don't really help me understand when you'd want to use this 
> > > > > attribute. Perhaps a link to what JSPI is and a code example would 
> > > > > help a little bit? Or is this more of a low-level implementation 
> > > > > detail kind of attribute where folks already know the domain?
> > > > Based on the documentation here, I'm wondering why the `annotate` 
> > > > attribute doesn't suffice? That attribute lets you specify custom 
> > > > information to associate with a declaration that then gets lowered such 
> > > > that passes can do whatever they want with the info, which seems to be 
> > > > a more generalized version of what this attribute is.
> > > > 
> > > > (FWIW, I'm back to having basically no idea when you'd use this 
> > > > attribute or what it would be used for, so my thoughts above might make 
> > > > no sense.)
> > > I was considering that, but it would require more machinery in the wasm 
> > > backend to store all the attribute values in the output. For now we only 
> > > really need a flag associated with function. I think if we find more uses 
> > > for the annotations in the future we could replace wasm_custom with it.
> > > I was considering that, but it would require more machinery in the wasm 
> > > backend to store all the attribute

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-22 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Marking as requested changes so it's clear there's more worth discussing, so we 
don't accidentally land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-22 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

brendandahl wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > This could be my ignorance of web assembly showing, but the docs don't 
> > > really help me understand when you'd want to use this attribute. Perhaps 
> > > a link to what JSPI is and a code example would help a little bit? Or is 
> > > this more of a low-level implementation detail kind of attribute where 
> > > folks already know the domain?
> > Based on the documentation here, I'm wondering why the `annotate` attribute 
> > doesn't suffice? That attribute lets you specify custom information to 
> > associate with a declaration that then gets lowered such that passes can do 
> > whatever they want with the info, which seems to be a more generalized 
> > version of what this attribute is.
> > 
> > (FWIW, I'm back to having basically no idea when you'd use this attribute 
> > or what it would be used for, so my thoughts above might make no sense.)
> I was considering that, but it would require more machinery in the wasm 
> backend to store all the attribute values in the output. For now we only 
> really need a flag associated with function. I think if we find more uses for 
> the annotations in the future we could replace wasm_custom with it.
> I was considering that, but it would require more machinery in the wasm 
> backend to store all the attribute values in the output. For now we only 
> really need a flag associated with function. I think if we find more uses for 
> the annotations in the future we could replace wasm_custom with it.

More machinery in the backend is preferred to exposing a new attribute that is 
this general-purpose; the backend is what needs this functionality, the 
frontend basically does nothing with it. (I'm assuming this is an 
implementation detail attribute and not something you expect users to write. If 
I'm wrong about that, please let me know.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-21 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

aaron.ballman wrote:
> aaron.ballman wrote:
> > This could be my ignorance of web assembly showing, but the docs don't 
> > really help me understand when you'd want to use this attribute. Perhaps a 
> > link to what JSPI is and a code example would help a little bit? Or is this 
> > more of a low-level implementation detail kind of attribute where folks 
> > already know the domain?
> Based on the documentation here, I'm wondering why the `annotate` attribute 
> doesn't suffice? That attribute lets you specify custom information to 
> associate with a declaration that then gets lowered such that passes can do 
> whatever they want with the info, which seems to be a more generalized 
> version of what this attribute is.
> 
> (FWIW, I'm back to having basically no idea when you'd use this attribute or 
> what it would be used for, so my thoughts above might make no sense.)
I was considering that, but it would require more machinery in the wasm backend 
to store all the attribute values in the output. For now we only really need a 
flag associated with function. I think if we find more uses for the annotations 
in the future we could replace wasm_custom with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-21 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5608-5612
+Clang supports the ``__attribute__((wasm_async))``
+attribute for the WebAssembly target. This attribute may be attached to a
+function definition, which indicates the function will be used with JavaScript
+promise integration (JSPI). The attribute will cause the creation of a custom
+section named "async" that contains each wasm_async function's index value.

aaron.ballman wrote:
> This could be my ignorance of web assembly showing, but the docs don't really 
> help me understand when you'd want to use this attribute. Perhaps a link to 
> what JSPI is and a code example would help a little bit? Or is this more of a 
> low-level implementation detail kind of attribute where folks already know 
> the domain?
Based on the documentation here, I'm wondering why the `annotate` attribute 
doesn't suffice? That attribute lets you specify custom information to 
associate with a declaration that then gets lowered such that passes can do 
whatever they want with the info, which seems to be a more generalized version 
of what this attribute is.

(FWIW, I'm back to having basically no idea when you'd use this attribute or 
what it would be used for, so my thoughts above might make no sense.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-20 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl added a comment.

@aaron.ballman or @erichkeane Did you want to re-review after that latest 
changes (more generic attribute) or are things good to go?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-20 Thread Dan Gohman via Phabricator via cfe-commits 
sunfish added a comment.

In D150803#4419436 , @sbc100 wrote:

> Did you mean to comment on the old PR?  This new PR doesn't propose either 
> "jspi" or "async", but adds that ability to define custom attributes.. since 
> that was deemed more flexible and forward thinking.   The decision as to 
> which name emscripten will use for what can then be part of  different 
> discussion/PR we hope.

Ah, I think I somehow was looking at the old PR, and thought there was a test 
with `custom_section.func_attr.async` in it.

This patch looks ok, though my initial impression is that if we're going to 
have a fully general-purpose mechanism like this, we might want to plan ahead 
for attributes that have parameters. We don't need to implement it now though; 
if there's a reasonable way to evolve in that direction, then this LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-14 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl updated this revision to Diff 531390.
brendandahl marked an inline comment as done.
brendandahl added a comment.

Review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

Files:
  clang/docs/ReleaseNotes.rst
  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-custom-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-wasm-custom.c
  clang/test/Sema/wasm-custom-support.c
  lld/test/wasm/custom-attr-tombstone.s
  lld/test/wasm/custom-attr.s
  lld/test/wasm/merge-custom-attr-section.s
  lld/wasm/InputChunks.cpp
  lld/wasm/InputFiles.cpp
  llvm/include/llvm/BinaryFormat/WasmRelocs.def
  llvm/include/llvm/MC/MCExpr.h
  llvm/lib/MC/MCExpr.cpp
  llvm/lib/MC/WasmObjectWriter.cpp
  llvm/lib/Object/WasmObjectFile.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
  llvm/test/CodeGen/WebAssembly/custom-attr.ll
  llvm/test/MC/WebAssembly/custom-attr.s

Index: llvm/test/MC/WebAssembly/custom-attr.s
===
--- /dev/null
+++ llvm/test/MC/WebAssembly/custom-attr.s
@@ -0,0 +1,21 @@
+# RUN: llvm-mc -triple=wasm32-unknown-unknown < %s | FileCheck %s
+# Check that it also comiled to object for format.
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -o - < %s | obj2yaml | FileCheck -check-prefix=CHECK-OBJ %s
+
+foo:
+.globl foo
+.functype foo () -> ()
+end_function
+
+.section.custom_section.func_attr.custom0,"",@
+.int32  foo@FUNCINDEX
+
+# CHECK:   .section .custom_section.func_attr.custom0,"",@
+# CHECK-NEXT: .int32  foo@FUNCINDEX
+
+# CHECK-OBJ:- Type:CUSTOM
+# CHECK-OBJ-NEXT: Relocations:
+# CHECK-OBJ-NEXT:- Type:R_WASM_FUNCTION_INDEX_I32
+# CHECK-OBJ-NEXT:  Index:   0
+# CHECK-OBJ-NEXT:  Offset:  0x0
+# CHECK-OBJ-NEXT: Name:func_attr.custom0
Index: llvm/test/CodeGen/WebAssembly/custom-attr.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/custom-attr.ll
@@ -0,0 +1,24 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+define void @test0() #0 {
+  ret void
+}
+
+define void @test1() #0 {
+  ret void
+}
+
+define void @test3() #1 {
+  ret void
+}
+
+attributes #0 = { "wasm-custom"="custom0" }
+attributes #1 = { "wasm-custom"="custom1" }
+
+; CHECK:  .section.custom_section.func_attr.custom0,"",@
+; CHECK-NEXT: .int32  test0@FUNCINDEX
+; CHECK-NEXT: .int32  test1@FUNCINDEX
+; CHECK:  .section.custom_section.func_attr.custom1,"",@
+; CHECK-NEXT: .int32  test3@FUNCINDEX
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
@@ -66,6 +66,7 @@
   void emitEndOfAsmFile(Module ) override;
   void EmitProducerInfo(Module );
   void EmitTargetFeatures(Module );
+  void EmitCustomFunctionAttributes(Module );
   void emitSymbolType(const MCSymbolWasm *Sym);
   void emitGlobalVariable(const GlobalVariable *GV) override;
   void emitJumpTableInfo() override;
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -25,6 +25,7 @@
 #include "WebAssemblyRegisterInfo.h"
 #include "WebAssemblyRuntimeLibcallSignatures.h"
 #include "WebAssemblyTargetMachine.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/BinaryFormat/Wasm.h"
@@ -438,6 +439,7 @@
 
   EmitProducerInfo(M);
   EmitTargetFeatures(M);
+  EmitCustomFunctionAttributes(M);
 }
 
 void WebAssemblyAsmPrinter::EmitProducerInfo(Module ) {
@@ -556,6 +558,34 @@
   OutStreamer->popSection();
 }
 
+void WebAssemblyAsmPrinter::EmitCustomFunctionAttributes(Module ) {
+  DenseMap> CustomSections;
+  // Group all the custom attributes by name.
+  for (const auto  : M) {
+auto *Sym = cast(getSymbol());
+if (F.hasFnAttribute("wasm-custom")) {
+  StringRef Name = F.getFnAttribute("wasm-custom").getValueAsString();
+  CustomSections[Name].push_back(Sym);
+}
+  }
+
+  // Emit a custom section for each unique attribute.
+  for (auto [Name, Symbols] : CustomSections) {
+MCSectionWasm *CustomSection = OutContext.getWasmSection(
+".custom_section.func_attr." +

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-14 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl marked 4 inline comments as done.
brendandahl added inline comments.



Comment at: lld/test/wasm/custom-undefine.s:17
+.type   bar,@function
+bar:
+.functype   bar () -> ()

dschuff wrote:
> I don't fully understand how this test is different from custom-attr.s. Bar 
> is still defined, so the only thing I can see that's different is that it's 
> not called. Is it relying on the linker GCing bar so that it's not defined? 
> is the result different when bar is really undefined?
Undefined is a bad name. I'll change to unreferenced or something. This tests 
when the function isn't referenced a tombstone will be placed in the custom 
section (e.g. the payload below has a ).



Comment at: lld/test/wasm/merge-custom-attr-section.ll:47
+}
+
+attributes #0 = { "wasm-custom"="custom0" }

sbc100 wrote:
> Is there some reason we can't use assembly for this test?
Nope, I rewrote the other two, but missed this one. I'll fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-13 Thread Sam Clegg via Phabricator via cfe-commits 
sbc100 added a comment.

In D150803#4419370 , @sunfish wrote:

> Would it make sense to call this "jspi" in practice, rather than "async"? 
> Even though this isn't clang's decision per se anymore, it seems like the 
> reasoning earlier in this review would still apply.

Did you mean to comment on the old PR?  This new PR doesn't propose either 
"jspi" or "async", but adds that ability to define custom attributes.. since 
that was deemed more flexible and forward thinking.   The decision as to which 
name emscripten will use for what can then be part of  different discussion/PR 
we hope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-13 Thread Dan Gohman via Phabricator via cfe-commits 
sunfish added a comment.

Would it make sense to call this "jspi" in practice, rather than "async"? Even 
though this isn't clang's decision per se anymore, it seems like the reasoning 
earlier in this review would still apply.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-13 Thread Sam Clegg via Phabricator via cfe-commits 
sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

Nice!




Comment at: lld/test/wasm/merge-custom-attr-section.ll:47
+}
+
+attributes #0 = { "wasm-custom"="custom0" }

Is there some reason we can't use assembly for this test?



Comment at: lld/wasm/InputChunks.cpp:530
   if (!name.startswith(".debug_"))
 return 0;
   if (name.equals(".debug_ranges") || name.equals(".debug_loc"))

Can we switch the logic around so that the default `return 0;` is that last 
line of the function.   Then your new use case could be added right before that 
line.

e.g. 
```
 if (name.startswith(".debug_"))
return UINT64_C(-1)
```




Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:569
+  if (!CustomSections.contains(Name))
+CustomSections[Name] = {};
+  CustomSections[Name].push_back(Sym);

Are these two lines needed?  i.e. does the subscript operator automatically 
create an empty element when its first used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-13 Thread Derek Schuff via Phabricator via cfe-commits 
dschuff added a comment.

oh also: please add the new relocation type to the document at 
https://github.com/WebAssembly/tool-conventions/blob/main/Linking.md


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-13 Thread Derek Schuff via Phabricator via cfe-commits 
dschuff added a comment.

backend code looks pretty good. I'll defer to the clang experts for the clang 
part.




Comment at: lld/test/wasm/custom-undefine.s:17
+.type   bar,@function
+bar:
+.functype   bar () -> ()

I don't fully understand how this test is different from custom-attr.s. Bar is 
still defined, so the only thing I can see that's different is that it's not 
called. Is it relying on the linker GCing bar so that it's not defined? is the 
result different when bar is really undefined?



Comment at: lld/test/wasm/merge-custom-attr-section.ll:1
+; RUN: split-file %s %t
+; RUN: llc -filetype=obj %t/a.ll -o %t1.o

wow I didn't know this utility existed 勞



Comment at: lld/test/wasm/merge-custom-attr-section.ll:7
+
+; Ensure two custo funct_attr sections are concatenated together.
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

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

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

2023-06-13 Thread Brendan Dahl via Phabricator via cfe-commits 
brendandahl updated this revision to Diff 531014.
brendandahl added a comment.

Fix few remaining issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803

Files:
  clang/docs/ReleaseNotes.rst
  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-custom-attr.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-wasm-custom.c
  clang/test/Sema/wasm-custom-support.c
  lld/test/wasm/custom-attr.s
  lld/test/wasm/custom-undefine.s
  lld/test/wasm/merge-custom-attr-section.ll
  lld/wasm/InputChunks.cpp
  lld/wasm/InputFiles.cpp
  llvm/include/llvm/BinaryFormat/WasmRelocs.def
  llvm/include/llvm/MC/MCExpr.h
  llvm/lib/MC/MCExpr.cpp
  llvm/lib/MC/WasmObjectWriter.cpp
  llvm/lib/Object/WasmObjectFile.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
  llvm/test/CodeGen/WebAssembly/custom-attr.ll
  llvm/test/MC/WebAssembly/custom-attr.s

Index: llvm/test/MC/WebAssembly/custom-attr.s
===
--- /dev/null
+++ llvm/test/MC/WebAssembly/custom-attr.s
@@ -0,0 +1,21 @@
+# RUN: llvm-mc -triple=wasm32-unknown-unknown < %s | FileCheck %s
+# Check that it also comiled to object for format.
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -filetype=obj -o - < %s | obj2yaml | FileCheck -check-prefix=CHECK-OBJ %s
+
+foo:
+.globl foo
+.functype foo () -> ()
+end_function
+
+.section.custom_section.func_attr.custom0,"",@
+.int32  foo@FUNCINDEX
+
+# CHECK:   .section .custom_section.func_attr.custom0,"",@
+# CHECK-NEXT: .int32  foo@FUNCINDEX
+
+# CHECK-OBJ:- Type:CUSTOM
+# CHECK-OBJ-NEXT: Relocations:
+# CHECK-OBJ-NEXT:- Type:R_WASM_FUNCTION_INDEX_I32
+# CHECK-OBJ-NEXT:  Index:   0
+# CHECK-OBJ-NEXT:  Offset:  0x0
+# CHECK-OBJ-NEXT: Name:func_attr.custom0
Index: llvm/test/CodeGen/WebAssembly/custom-attr.ll
===
--- /dev/null
+++ llvm/test/CodeGen/WebAssembly/custom-attr.ll
@@ -0,0 +1,24 @@
+; RUN: llc < %s -asm-verbose=false -wasm-keep-registers | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+define void @test0() #0 {
+  ret void
+}
+
+define void @test1() #0 {
+  ret void
+}
+
+define void @test3() #1 {
+  ret void
+}
+
+attributes #0 = { "wasm-custom"="custom0" }
+attributes #1 = { "wasm-custom"="custom1" }
+
+; CHECK:  .section.custom_section.func_attr.custom0,"",@
+; CHECK-NEXT: .int32  test0@FUNCINDEX
+; CHECK-NEXT: .int32  test1@FUNCINDEX
+; CHECK:  .section.custom_section.func_attr.custom1,"",@
+; CHECK-NEXT: .int32  test3@FUNCINDEX
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h
@@ -66,6 +66,7 @@
   void emitEndOfAsmFile(Module ) override;
   void EmitProducerInfo(Module );
   void EmitTargetFeatures(Module );
+  void EmitCustomFunctionAttributes(Module );
   void emitSymbolType(const MCSymbolWasm *Sym);
   void emitGlobalVariable(const GlobalVariable *GV) override;
   void emitJumpTableInfo() override;
Index: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
@@ -25,6 +25,7 @@
 #include "WebAssemblyRegisterInfo.h"
 #include "WebAssemblyRuntimeLibcallSignatures.h"
 #include "WebAssemblyTargetMachine.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/BinaryFormat/Wasm.h"
@@ -438,6 +439,7 @@
 
   EmitProducerInfo(M);
   EmitTargetFeatures(M);
+  EmitCustomFunctionAttributes(M);
 }
 
 void WebAssemblyAsmPrinter::EmitProducerInfo(Module ) {
@@ -556,6 +558,36 @@
   OutStreamer->popSection();
 }
 
+void WebAssemblyAsmPrinter::EmitCustomFunctionAttributes(Module ) {
+  DenseMap> CustomSections;
+  // Group all the custom attributes by name.
+  for (const auto  : M) {
+auto *Sym = cast(getSymbol());
+if (F.hasFnAttribute("wasm-custom")) {
+  StringRef Name = F.getFnAttribute("wasm-custom").getValueAsString();
+  if (!CustomSections.contains(Name))
+CustomSections[Name] = {};
+  CustomSections[Name].push_back(Sym);
+}
+  }
+
+  // Emit a custom section for each unique attribute.
+  for (auto [Name, Symbols] : CustomSections) {
+MCSectionWasm *CustomSection = OutContext.getWasmSection(
+