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

Thu Jun 22 09:50:28 PDT 2023

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 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 (2) today and we should probably wait until we have a use case before designed a more complex version of this attribute.
(1) could also be more complex since annotate takes a name and optional additional arguments e.g.` __attribute__((annotate("x", "arg1", "arg2")))`. I suppose we could just handle the case where there's a name for now and effectively do what this patch is currently doing.

================
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:
> 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 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 (2) today and we should probably wait until we have a use case before designed a more complex version of this attribute.
> (1) could also be more complex since annotate takes a name and optional additional arguments e.g.` __attribute__((annotate("x", "arg1", "arg2")))`. I suppose we could just handle the case where there's a name for now and effectively do what this patch is currently doing.
It's been awhile, but I think I originally didn't go with annotate since 1) we thought it shouldn't affect compilation (in practice that doesn't seem to be true anymore) 2) It was more complex (e.g. args mentioned above). I'll give using annotate with a single attribute a try. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803