Re: r352930 - [WebAssembly] Add an import_field function attribute
On Mon, Mar 18, 2019 at 7:08 PM Dan Gohman wrote: > > > > On Fri, Mar 15, 2019 at 10:55 AM Aaron Ballman wrote: >> >> >> Ping. > > > I apologize for the extraordinarily delays here. I've now posted a patch to > address the review comments here: > > https://reviews.llvm.org/D59520 Delays happen to all of us; thank you for addressing the comments -- I appreciate it! ~Aaron > > Dan > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Mar 15, 2019 at 10:55 AM Aaron Ballman wrote: > > Ping. > I apologize for the extraordinarily delays here. I've now posted a patch to address the review comments here: https://reviews.llvm.org/D59520 Dan ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Mar 8, 2019 at 5:06 PM Aaron Ballman wrote: > > On Wed, Feb 6, 2019 at 12:43 AM Dan Gohman wrote: > > > > > > > > On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman > > wrote: > >> > >> On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: > >> > > >> > > >> > Indeed, and the existing import_module attribute needs these tests as > >> > well. I'll write some and add them in a follow-up patch. > >> > > >> > Thanks for the review! > >> > >> Any time! I haven't seen that follow-up patch yet though; did it fall > >> off your radar? > > > > > > Yes, it's still on my radar. > > It's been a month and it still seems like this hasn't been taken care > of. The usual expectation is to handle post-commit feedback > "immediately". Can you please address these concerns? Ping. ~Aaron > > ~Aaron > > > > > Dan > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Wed, Feb 6, 2019 at 12:43 AM Dan Gohman wrote: > > > > On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman wrote: >> >> On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: >> > >> > >> > Indeed, and the existing import_module attribute needs these tests as >> > well. I'll write some and add them in a follow-up patch. >> > >> > Thanks for the review! >> >> Any time! I haven't seen that follow-up patch yet though; did it fall >> off your radar? > > > Yes, it's still on my radar. It's been a month and it still seems like this hasn't been taken care of. The usual expectation is to handle post-commit feedback "immediately". Can you please address these concerns? ~Aaron > > Dan > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Tue, Feb 5, 2019 at 11:20 AM Aaron Ballman wrote: > On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote: > > > > > > Indeed, and the existing import_module attribute needs these tests as > well. I'll write some and add them in a follow-up patch. > > > > Thanks for the review! > > Any time! I haven't seen that follow-up patch yet though; did it fall > off your radar? > Yes, it's still on my radar. Dan ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Feb 1, 2019 at 8:27 PM Dan Gohman wrote:
>
>
>
> On Fri, Feb 1, 2019 at 2:31 PM Aaron Ballman wrote:
>>
>> On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits
>> wrote:
>> >
>> > Author: djg
>> > Date: Fri Feb 1 14:25:23 2019
>> > New Revision: 352930
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=352930=rev
>> > Log:
>> > [WebAssembly] Add an import_field function attribute
>> >
>> > This is similar to import_module, but sets the import field name
>> > instead.
>> >
>> > By default, the import field name is the same as the C/asm/.o symbol
>> > name. However, there are situations where it's useful to have it be
>> > different. For example, suppose I have a wasm API with a module named
>> > "pwsix" and a field named "read". There's no risk of namespace
>> > collisions with user code at the wasm level because the generic name
>> > "read" is qualified by the module name "pwsix". However in the C/asm/.o
>> > namespaces, the module name is not used, so if I have a global function
>> > named "read", it is intruding on the user's namespace.
>> >
>> > With the import_field module, I can declare my function (in libc) to be
>> > "__read", and then set the wasm import module to be "pwsix" and the wasm
>> > import field to be "read". So at the C/asm/.o levels, my symbol is
>> > outside the user namespace.
>> >
>> > Differential Revision: https://reviews.llvm.org/D57602
>>
>> Btw, this review never went to cfe-commits, but it should have. :-)
>
>
> Yes, my mistake.
No worries, it seems there was a misbehaving phab script that did this
automatically for a few reviews, but it's been fixed.
>>
>>
>> > Added:
>> > cfe/trunk/test/CodeGen/wasm-import-name.c
>> > - copied, changed from r352781,
>> > cfe/trunk/test/CodeGen/wasm-import-module.c
>> > Modified:
>> > cfe/trunk/include/clang/Basic/Attr.td
>> > cfe/trunk/include/clang/Basic/AttrDocs.td
>> > cfe/trunk/lib/CodeGen/TargetInfo.cpp
>> > cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> > cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
>> >
>> > Modified: cfe/trunk/include/clang/Basic/Attr.td
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=352930=352929=352930=diff
>> > ==
>> > --- cfe/trunk/include/clang/Basic/Attr.td (original)
>> > +++ cfe/trunk/include/clang/Basic/Attr.td Fri Feb 1 14:25:23 2019
>> > @@ -1514,11 +1514,19 @@ def AMDGPUNumVGPR : InheritableAttr {
>> > def WebAssemblyImportModule : InheritableAttr,
>> >TargetSpecificAttr {
>> >let Spellings = [Clang<"import_module">];
>> > - let Args = [StringArgument<"ImportModuleName">];
>> > + let Args = [StringArgument<"ImportModule">];
>> >let Documentation = [WebAssemblyImportModuleDocs];
>> >let Subjects = SubjectList<[Function], ErrorDiag>;
>> > }
>> >
>> > +def WebAssemblyImportName : InheritableAttr,
>> > +TargetSpecificAttr {
>> > + let Spellings = [Clang<"import_name">];
>> > + let Args = [StringArgument<"ImportName">];
>> > + let Documentation = [WebAssemblyImportNameDocs];
>> > + let Subjects = SubjectList<[Function], ErrorDiag>;
>> > +}
>> > +
>> > def NoSplitStack : InheritableAttr {
>> >let Spellings = [GCC<"no_split_stack">];
>> >let Subjects = SubjectList<[Function], ErrorDiag>;
>> >
>> > Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=352930=352929=352930=diff
>> > ==
>> > --- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
>> > +++ cfe/trunk/include/clang/Basic/AttrDocs.td Fri Feb 1 14:25:23 2019
>> > @@ -3730,6 +3730,23 @@ reuqest a specific module name be used i
>> >}];
>> > }
>> >
>> > +def WebAssemblyImportNameDocs : Documentation {
>> > + let Category = DocCatFunction;
>> > + let Content = [{
>> > +Clang supports the ``__attribute__((import_name()))``
>> > +attribute for the WebAssembly target. This attribute may be attached to a
>> > +function declaration, where it modifies how the symbol is to be imported
>> > +within the WebAssembly linking environment.
>> > +
>> > +WebAssembly imports use a two-level namespace scheme, consisting of a
>> > module
>> > +name, which typically identifies a module from which to import, and a
>> > field
>> > +name, which typically identifies a field from that module to import. By
>> > +default, field names for C/C++ symbols are the same as their C/C++ symbol
>> > +names. This attribute can be used to override the default behavior, and
>> > +reuqest a specific field name be used instead.
>> > + }];
>> > +}
>> > +
>> > def ArtificialDocs : Documentation {
>> >let Category = DocCatFunction;
>> >let Content = [{
>> >
>> > Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
>> > URL:
>> >
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Feb 1, 2019 at 2:31 PM Aaron Ballman wrote:
> On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits
> wrote:
> >
> > Author: djg
> > Date: Fri Feb 1 14:25:23 2019
> > New Revision: 352930
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=352930=rev
> > Log:
> > [WebAssembly] Add an import_field function attribute
> >
> > This is similar to import_module, but sets the import field name
> > instead.
> >
> > By default, the import field name is the same as the C/asm/.o symbol
> > name. However, there are situations where it's useful to have it be
> > different. For example, suppose I have a wasm API with a module named
> > "pwsix" and a field named "read". There's no risk of namespace
> > collisions with user code at the wasm level because the generic name
> > "read" is qualified by the module name "pwsix". However in the C/asm/.o
> > namespaces, the module name is not used, so if I have a global function
> > named "read", it is intruding on the user's namespace.
> >
> > With the import_field module, I can declare my function (in libc) to be
> > "__read", and then set the wasm import module to be "pwsix" and the wasm
> > import field to be "read". So at the C/asm/.o levels, my symbol is
> > outside the user namespace.
> >
> > Differential Revision: https://reviews.llvm.org/D57602
>
> Btw, this review never went to cfe-commits, but it should have. :-)
>
Yes, my mistake.
>
> > Added:
> > cfe/trunk/test/CodeGen/wasm-import-name.c
> > - copied, changed from r352781,
> cfe/trunk/test/CodeGen/wasm-import-module.c
> > Modified:
> > cfe/trunk/include/clang/Basic/Attr.td
> > cfe/trunk/include/clang/Basic/AttrDocs.td
> > cfe/trunk/lib/CodeGen/TargetInfo.cpp
> > cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> > cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
> >
> > Modified: cfe/trunk/include/clang/Basic/Attr.td
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=352930=352929=352930=diff
> >
> ==
> > --- cfe/trunk/include/clang/Basic/Attr.td (original)
> > +++ cfe/trunk/include/clang/Basic/Attr.td Fri Feb 1 14:25:23 2019
> > @@ -1514,11 +1514,19 @@ def AMDGPUNumVGPR : InheritableAttr {
> > def WebAssemblyImportModule : InheritableAttr,
> >TargetSpecificAttr {
> >let Spellings = [Clang<"import_module">];
> > - let Args = [StringArgument<"ImportModuleName">];
> > + let Args = [StringArgument<"ImportModule">];
> >let Documentation = [WebAssemblyImportModuleDocs];
> >let Subjects = SubjectList<[Function], ErrorDiag>;
> > }
> >
> > +def WebAssemblyImportName : InheritableAttr,
> > +TargetSpecificAttr {
> > + let Spellings = [Clang<"import_name">];
> > + let Args = [StringArgument<"ImportName">];
> > + let Documentation = [WebAssemblyImportNameDocs];
> > + let Subjects = SubjectList<[Function], ErrorDiag>;
> > +}
> > +
> > def NoSplitStack : InheritableAttr {
> >let Spellings = [GCC<"no_split_stack">];
> >let Subjects = SubjectList<[Function], ErrorDiag>;
> >
> > Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=352930=352929=352930=diff
> >
> ==
> > --- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
> > +++ cfe/trunk/include/clang/Basic/AttrDocs.td Fri Feb 1 14:25:23 2019
> > @@ -3730,6 +3730,23 @@ reuqest a specific module name be used i
> >}];
> > }
> >
> > +def WebAssemblyImportNameDocs : Documentation {
> > + let Category = DocCatFunction;
> > + let Content = [{
> > +Clang supports the ``__attribute__((import_name()))``
> > +attribute for the WebAssembly target. This attribute may be attached to
> a
> > +function declaration, where it modifies how the symbol is to be imported
> > +within the WebAssembly linking environment.
> > +
> > +WebAssembly imports use a two-level namespace scheme, consisting of a
> module
> > +name, which typically identifies a module from which to import, and a
> field
> > +name, which typically identifies a field from that module to import. By
> > +default, field names for C/C++ symbols are the same as their C/C++
> symbol
> > +names. This attribute can be used to override the default behavior, and
> > +reuqest a specific field name be used instead.
> > + }];
> > +}
> > +
> > def ArtificialDocs : Documentation {
> >let Category = DocCatFunction;
> >let Content = [{
> >
> > Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=352930=352929=352930=diff
> >
> ==
> > --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
> > +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Fri Feb 1 14:25:23 2019
> >
Re: r352930 - [WebAssembly] Add an import_field function attribute
On Fri, Feb 1, 2019 at 5:25 PM Dan Gohman via cfe-commits
wrote:
>
> Author: djg
> Date: Fri Feb 1 14:25:23 2019
> New Revision: 352930
>
> URL: http://llvm.org/viewvc/llvm-project?rev=352930=rev
> Log:
> [WebAssembly] Add an import_field function attribute
>
> This is similar to import_module, but sets the import field name
> instead.
>
> By default, the import field name is the same as the C/asm/.o symbol
> name. However, there are situations where it's useful to have it be
> different. For example, suppose I have a wasm API with a module named
> "pwsix" and a field named "read". There's no risk of namespace
> collisions with user code at the wasm level because the generic name
> "read" is qualified by the module name "pwsix". However in the C/asm/.o
> namespaces, the module name is not used, so if I have a global function
> named "read", it is intruding on the user's namespace.
>
> With the import_field module, I can declare my function (in libc) to be
> "__read", and then set the wasm import module to be "pwsix" and the wasm
> import field to be "read". So at the C/asm/.o levels, my symbol is
> outside the user namespace.
>
> Differential Revision: https://reviews.llvm.org/D57602
Btw, this review never went to cfe-commits, but it should have. :-)
> Added:
> cfe/trunk/test/CodeGen/wasm-import-name.c
> - copied, changed from r352781,
> cfe/trunk/test/CodeGen/wasm-import-module.c
> Modified:
> cfe/trunk/include/clang/Basic/Attr.td
> cfe/trunk/include/clang/Basic/AttrDocs.td
> cfe/trunk/lib/CodeGen/TargetInfo.cpp
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
>
> Modified: cfe/trunk/include/clang/Basic/Attr.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=352930=352929=352930=diff
> ==
> --- cfe/trunk/include/clang/Basic/Attr.td (original)
> +++ cfe/trunk/include/clang/Basic/Attr.td Fri Feb 1 14:25:23 2019
> @@ -1514,11 +1514,19 @@ def AMDGPUNumVGPR : InheritableAttr {
> def WebAssemblyImportModule : InheritableAttr,
>TargetSpecificAttr {
>let Spellings = [Clang<"import_module">];
> - let Args = [StringArgument<"ImportModuleName">];
> + let Args = [StringArgument<"ImportModule">];
>let Documentation = [WebAssemblyImportModuleDocs];
>let Subjects = SubjectList<[Function], ErrorDiag>;
> }
>
> +def WebAssemblyImportName : InheritableAttr,
> +TargetSpecificAttr {
> + let Spellings = [Clang<"import_name">];
> + let Args = [StringArgument<"ImportName">];
> + let Documentation = [WebAssemblyImportNameDocs];
> + let Subjects = SubjectList<[Function], ErrorDiag>;
> +}
> +
> def NoSplitStack : InheritableAttr {
>let Spellings = [GCC<"no_split_stack">];
>let Subjects = SubjectList<[Function], ErrorDiag>;
>
> Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=352930=352929=352930=diff
> ==
> --- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
> +++ cfe/trunk/include/clang/Basic/AttrDocs.td Fri Feb 1 14:25:23 2019
> @@ -3730,6 +3730,23 @@ reuqest a specific module name be used i
>}];
> }
>
> +def WebAssemblyImportNameDocs : Documentation {
> + let Category = DocCatFunction;
> + let Content = [{
> +Clang supports the ``__attribute__((import_name()))``
> +attribute for the WebAssembly target. This attribute may be attached to a
> +function declaration, where it modifies how the symbol is to be imported
> +within the WebAssembly linking environment.
> +
> +WebAssembly imports use a two-level namespace scheme, consisting of a module
> +name, which typically identifies a module from which to import, and a field
> +name, which typically identifies a field from that module to import. By
> +default, field names for C/C++ symbols are the same as their C/C++ symbol
> +names. This attribute can be used to override the default behavior, and
> +reuqest a specific field name be used instead.
> + }];
> +}
> +
> def ArtificialDocs : Documentation {
>let Category = DocCatFunction;
>let Content = [{
>
> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=352930=352929=352930=diff
> ==
> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Fri Feb 1 14:25:23 2019
> @@ -765,7 +765,13 @@ public:
>if (const auto *Attr = FD->getAttr()) {
> llvm::Function *Fn = cast(GV);
> llvm::AttrBuilder B;
> -B.addAttribute("wasm-import-module", Attr->getImportModuleName());
> +B.addAttribute("wasm-import-module",
