SV: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
Hi! Thanks for the review. Having proper grammar in user facing text is important so I in no way feel it is overzealous. I'll push the patch once I fixed the points mentioned in the review. Regards, Petter Från: David Malcolm Skickat: den 13 december 2021 20:22 Till: Antoni Boucher; Petter Tomner; gcc-patches@gcc.gnu.org; j...@gcc.gnu.org Ämne: Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors On Sun, 2021-12-12 at 20:39 -0500, Antoni Boucher wrote: > Yes, this patch works for rustc_codegen_gcc perfectly. > It even fixes one issue that was in my patch, so that's nice! Excellent - thanks Antoni. > > Le samedi 11 décembre 2021 à 15:35 +, Petter Tomner a écrit : > > Hi! > > > > > s/an union/a union/ > > > s/a rvalue/an rvalue/ > > > > Heh no way ... and I though I knew English grammar :) I apologize for the non-regularity of English :) [and it's somewhat unfair for me to nit-pick, given that it's my first language - sorry; we may as well get this stuff correct] > > > > Had to look that up to see what the deal was but it makes sense. > > > > yunion, arevalue. Exactly. English spelling is a mess. > > > > > s/wrong-field-name/wrong-field-obj/ > > > > > > to match the struct example (given that the issue being tested > > > for > > > is > > > that it's the wrong object, rather than the wrong name). > > > > Initially, before submitting to the list, I made the code such that > > the field > > objects did not have to be the ones that were used when creating > > the > > struct or union, and forgot changing the test names. > > > > I figured it required too much string compares for the field names > > and > > pointer compares for the field object were more appropriate. To > > create > > dummy field objects were also kinda heavy. > > > > I'll address the points. Petter, you can go ahead and commit this, once you address the various points from my earlier review, and from Antoni's observation that "flags" should be "m_flags" when it's a member. Antoni has committed at least one ABI version bump since you posted this, so you'll need to rebase your work on top of his and adjust accordingly. Please re-test the patch before pushing it. Thanks for all your great work on this. Dave
Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
On Sun, 2021-12-12 at 20:39 -0500, Antoni Boucher wrote: > Yes, this patch works for rustc_codegen_gcc perfectly. > It even fixes one issue that was in my patch, so that's nice! Excellent - thanks Antoni. > > Le samedi 11 décembre 2021 à 15:35 +, Petter Tomner a écrit : > > Hi! > > > > > s/an union/a union/ > > > s/a rvalue/an rvalue/ > > > > Heh no way ... and I though I knew English grammar :) I apologize for the non-regularity of English :) [and it's somewhat unfair for me to nit-pick, given that it's my first language - sorry; we may as well get this stuff correct] > > > > Had to look that up to see what the deal was but it makes sense. > > > > yunion, arevalue. Exactly. English spelling is a mess. > > > > > s/wrong-field-name/wrong-field-obj/ > > > > > > to match the struct example (given that the issue being tested > > > for > > > is > > > that it's the wrong object, rather than the wrong name). > > > > Initially, before submitting to the list, I made the code such that > > the field > > objects did not have to be the ones that were used when creating > > the > > struct or union, and forgot changing the test names. > > > > I figured it required too much string compares for the field names > > and > > pointer compares for the field object were more appropriate. To > > create > > dummy field objects were also kinda heavy. > > > > I'll address the points. Petter, you can go ahead and commit this, once you address the various points from my earlier review, and from Antoni's observation that "flags" should be "m_flags" when it's a member. Antoni has committed at least one ABI version bump since you posted this, so you'll need to rebase your work on top of his and adjust accordingly. Please re-test the patch before pushing it. Thanks for all your great work on this. Dave
Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
Also, the LIBGCCJIT_ABI_17 will need to be updated as I merged some of my patches. Le samedi 11 décembre 2021 à 15:35 +, Petter Tomner a écrit : > Hi! > > > s/an union/a union/ > > s/a rvalue/an rvalue/ > > Heh no way ... and I though I knew English grammar :) > > Had to look that up to see what the deal was but it makes sense. > > yunion, arevalue. > > > s/wrong-field-name/wrong-field-obj/ > > > > to match the struct example (given that the issue being tested for > > is > > that it's the wrong object, rather than the wrong name). > > Initially, before submitting to the list, I made the code such that > the field > objects did not have to be the ones that were used when creating the > struct or union, and forgot changing the test names. > > I figured it required too much string compares for the field names > and > pointer compares for the field object were more appropriate. To > create > dummy field objects were also kinda heavy. > > I'll address the points. > > Regards, Petter > > > Från: David Malcolm > Skickat: den 9 december 2021 20:39 > Till: Petter Tomner; gcc-patches@gcc.gnu.org; j...@gcc.gnu.org; Antoni > Boucher > Ämne: Re: [PATCH v2] jit: Add support for global rvalue > initialization and ctors > > On Mon, 2021-12-06 at 10:47 +, Petter Tomner via Gcc-patches > wrote: > > Hi! > > > > Attached is a patch with changes in line with the review of the > > prior > > patch. > > The patch adds support for initialization of global variables with > > rvalues as well > > as rvalue constructors for structs, arrays and unions. > > Thanks for the updated patch. > > Antoni: does this patch work for you for your rustc plugin? > > > > > Review: https://gcc.gnu.org/pipermail/jit/2021q4/001400.html > > > > The points have been addressed, except: > > > > > Can the type be made const? > > > > I started to make types_kinda_same_internal () taking const args, > > but I > > felt the > > patch was ballooning because some spread out methods needed a const > > signature too. I could submit that in a separate patch. > > Fair enough; fixing that isn't a blocker; it's already a big patch. > > > > > I also addressed a problem Antoni found: > > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html > > > > , where you could not initialize global pointer variables to point > > to > > uninitialized variables. I did that by > > removing a redundant check with validate_var_has_init (), since > > that > > walking function would > > have to be quite complex to allow pointers to uninitialized > > variables. > > > > Any: > > const int foo; > > int bar = foo; > > > > will instead be reported as "not compile time constant" instead of > > a > > nice error message with names. > > > > make check-jit runs fine on gnu-linux-x64 Debian. > > Various review comments inline below, which are mostly just nits: > > > From a4fef1308eaa72ce4ec51dbe5efcfbbf032e9870 Mon Sep 17 00:00:00 > > 2001 > > From: Petter Tomner > > Date: Mon, 29 Nov 2021 20:44:07 +0100 > > Subject: [PATCH] Add support for global rvalue initialization and > > constructors > > > > This patch adds support for initialization of global variables > > with rvalues and creating constructors for array, struct and > > union types which can be used as rvalues. > > > > Signed-off-by: > > 2021-12-06 Petter Tomner > > [...snip...] > > > diff --git a/gcc/jit/docs/topics/expressions.rst > > b/gcc/jit/docs/topics/expressions.rst > > index 396259ef07e..5f64ca68595 100644 > > --- a/gcc/jit/docs/topics/expressions.rst > > +++ b/gcc/jit/docs/topics/expressions.rst > > @@ -126,6 +126,147 @@ Simple expressions > > underlying string, so it is valid to pass in a pointer to an > > on-stack > > buffer. > > > > +Constructor expressions > > +*** > > + > > + The following functions make constructors for array, struct and > > union > > + types. > > + > > + The constructor rvalue can be used for assignment to locals. > > + It can be used to initialize global variables with > > + :func:`gcc_jit_global_set_initializer_rvalue`. It can also be > > used as a > > + temporary value for function calls and return values, but its > > address > > + can't be taken. > > + > > + Note that arrays in libgccjit does not collapse to pointers > > like in > > s/does not/do not/ > > > + C. I.e. if an array constructor is used as e.g. a return value, > > the whole > > + array would be returned by value - array constructors can be > > assigned to > > + array variables. > > + > > + The constructor can contain nested constructors. > > + > > + Note that a string literal rvalue can't be used to construct a > > char array. > > + It need one rvalue for each char. > > s/char array. It need one rvalue/char array; the latter needs one > rvalue/ > > > + > > + These entrypoints were added in :ref:`LIBGCCJIT_ABI_16`; you > > can test for its > > s/16/17/ I believe. > > s/its/their/ > > > > + presense using: > > s
Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
One thing I'm not sure if it is a code style issue, but worth mentionning: > @@ -1405,8 +1436,10 @@ private: > > private: >enum gcc_jit_global_kind m_kind; > + enum global_var_flags flags = GLOBAL_VAR_FLAGS_NONE; ^ Should it be named m_flags instead of flags? >string *m_name; >void *m_initializer; > + rvalue *m_rvalue_init = nullptr; /* Only needed for write_dump. */ >size_t m_initializer_num_bytes; > }; Le samedi 11 décembre 2021 à 15:35 +, Petter Tomner a écrit : > Hi! > > > s/an union/a union/ > > s/a rvalue/an rvalue/ > > Heh no way ... and I though I knew English grammar :) > > Had to look that up to see what the deal was but it makes sense. > > yunion, arevalue. > > > s/wrong-field-name/wrong-field-obj/ > > > > to match the struct example (given that the issue being tested for > > is > > that it's the wrong object, rather than the wrong name). > > Initially, before submitting to the list, I made the code such that > the field > objects did not have to be the ones that were used when creating the > struct or union, and forgot changing the test names. > > I figured it required too much string compares for the field names > and > pointer compares for the field object were more appropriate. To > create > dummy field objects were also kinda heavy. > > I'll address the points. > > Regards, Petter > > > Från: David Malcolm > Skickat: den 9 december 2021 20:39 > Till: Petter Tomner; gcc-patches@gcc.gnu.org; j...@gcc.gnu.org; Antoni > Boucher > Ämne: Re: [PATCH v2] jit: Add support for global rvalue > initialization and ctors > > On Mon, 2021-12-06 at 10:47 +, Petter Tomner via Gcc-patches > wrote: > > Hi! > > > > Attached is a patch with changes in line with the review of the > > prior > > patch. > > The patch adds support for initialization of global variables with > > rvalues as well > > as rvalue constructors for structs, arrays and unions. > > Thanks for the updated patch. > > Antoni: does this patch work for you for your rustc plugin? > > > > > Review: https://gcc.gnu.org/pipermail/jit/2021q4/001400.html > > > > The points have been addressed, except: > > > > > Can the type be made const? > > > > I started to make types_kinda_same_internal () taking const args, > > but I > > felt the > > patch was ballooning because some spread out methods needed a const > > signature too. I could submit that in a separate patch. > > Fair enough; fixing that isn't a blocker; it's already a big patch. > > > > > I also addressed a problem Antoni found: > > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html > > > > , where you could not initialize global pointer variables to point > > to > > uninitialized variables. I did that by > > removing a redundant check with validate_var_has_init (), since > > that > > walking function would > > have to be quite complex to allow pointers to uninitialized > > variables. > > > > Any: > > const int foo; > > int bar = foo; > > > > will instead be reported as "not compile time constant" instead of > > a > > nice error message with names. > > > > make check-jit runs fine on gnu-linux-x64 Debian. > > Various review comments inline below, which are mostly just nits: > > > From a4fef1308eaa72ce4ec51dbe5efcfbbf032e9870 Mon Sep 17 00:00:00 > > 2001 > > From: Petter Tomner > > Date: Mon, 29 Nov 2021 20:44:07 +0100 > > Subject: [PATCH] Add support for global rvalue initialization and > > constructors > > > > This patch adds support for initialization of global variables > > with rvalues and creating constructors for array, struct and > > union types which can be used as rvalues. > > > > Signed-off-by: > > 2021-12-06 Petter Tomner > > [...snip...] > > > diff --git a/gcc/jit/docs/topics/expressions.rst > > b/gcc/jit/docs/topics/expressions.rst > > index 396259ef07e..5f64ca68595 100644 > > --- a/gcc/jit/docs/topics/expressions.rst > > +++ b/gcc/jit/docs/topics/expressions.rst > > @@ -126,6 +126,147 @@ Simple expressions > > underlying string, so it is valid to pass in a pointer to an > > on-stack > > buffer. > > > > +Constructor expressions > > +*** > > + > > + The following functions make constructors for array, struct and > > union > > + types. > > + > > + The constructor rvalue can be used for assignment to locals. > > + It can be used to initialize global variables with > > + :func:`gcc_jit_global_set_initializer_rvalue`. It can also be > > used as a > > + temporary value for function calls and return values, but its > > address > > + can't be taken. > > + > > + Note that arrays in libgccjit does not collapse to pointers > > like in > > s/does not/do not/ > > > + C. I.e. if an array constructor is used as e.g. a return value, > > the whole > > + array would be returned by value - array constructors can be > > assigned to > > + array variables. > > + > > + The constructor can contain nested constructors. > > + > > +
Re: SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
Yes, this patch works for rustc_codegen_gcc perfectly. It even fixes one issue that was in my patch, so that's nice! Le samedi 11 décembre 2021 à 15:35 +, Petter Tomner a écrit : > Hi! > > > s/an union/a union/ > > s/a rvalue/an rvalue/ > > Heh no way ... and I though I knew English grammar :) > > Had to look that up to see what the deal was but it makes sense. > > yunion, arevalue. > > > s/wrong-field-name/wrong-field-obj/ > > > > to match the struct example (given that the issue being tested for > > is > > that it's the wrong object, rather than the wrong name). > > Initially, before submitting to the list, I made the code such that > the field > objects did not have to be the ones that were used when creating the > struct or union, and forgot changing the test names. > > I figured it required too much string compares for the field names > and > pointer compares for the field object were more appropriate. To > create > dummy field objects were also kinda heavy. > > I'll address the points. > > Regards, Petter > > > Från: David Malcolm > Skickat: den 9 december 2021 20:39 > Till: Petter Tomner; gcc-patches@gcc.gnu.org; j...@gcc.gnu.org; Antoni > Boucher > Ämne: Re: [PATCH v2] jit: Add support for global rvalue > initialization and ctors > > On Mon, 2021-12-06 at 10:47 +, Petter Tomner via Gcc-patches > wrote: > > Hi! > > > > Attached is a patch with changes in line with the review of the > > prior > > patch. > > The patch adds support for initialization of global variables with > > rvalues as well > > as rvalue constructors for structs, arrays and unions. > > Thanks for the updated patch. > > Antoni: does this patch work for you for your rustc plugin? > > > > > Review: https://gcc.gnu.org/pipermail/jit/2021q4/001400.html > > > > The points have been addressed, except: > > > > > Can the type be made const? > > > > I started to make types_kinda_same_internal () taking const args, > > but I > > felt the > > patch was ballooning because some spread out methods needed a const > > signature too. I could submit that in a separate patch. > > Fair enough; fixing that isn't a blocker; it's already a big patch. > > > > > I also addressed a problem Antoni found: > > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html > > > > , where you could not initialize global pointer variables to point > > to > > uninitialized variables. I did that by > > removing a redundant check with validate_var_has_init (), since > > that > > walking function would > > have to be quite complex to allow pointers to uninitialized > > variables. > > > > Any: > > const int foo; > > int bar = foo; > > > > will instead be reported as "not compile time constant" instead of > > a > > nice error message with names. > > > > make check-jit runs fine on gnu-linux-x64 Debian. > > Various review comments inline below, which are mostly just nits: > > > From a4fef1308eaa72ce4ec51dbe5efcfbbf032e9870 Mon Sep 17 00:00:00 > > 2001 > > From: Petter Tomner > > Date: Mon, 29 Nov 2021 20:44:07 +0100 > > Subject: [PATCH] Add support for global rvalue initialization and > > constructors > > > > This patch adds support for initialization of global variables > > with rvalues and creating constructors for array, struct and > > union types which can be used as rvalues. > > > > Signed-off-by: > > 2021-12-06 Petter Tomner > > [...snip...] > > > diff --git a/gcc/jit/docs/topics/expressions.rst > > b/gcc/jit/docs/topics/expressions.rst > > index 396259ef07e..5f64ca68595 100644 > > --- a/gcc/jit/docs/topics/expressions.rst > > +++ b/gcc/jit/docs/topics/expressions.rst > > @@ -126,6 +126,147 @@ Simple expressions > > underlying string, so it is valid to pass in a pointer to an > > on-stack > > buffer. > > > > +Constructor expressions > > +*** > > + > > + The following functions make constructors for array, struct and > > union > > + types. > > + > > + The constructor rvalue can be used for assignment to locals. > > + It can be used to initialize global variables with > > + :func:`gcc_jit_global_set_initializer_rvalue`. It can also be > > used as a > > + temporary value for function calls and return values, but its > > address > > + can't be taken. > > + > > + Note that arrays in libgccjit does not collapse to pointers > > like in > > s/does not/do not/ > > > + C. I.e. if an array constructor is used as e.g. a return value, > > the whole > > + array would be returned by value - array constructors can be > > assigned to > > + array variables. > > + > > + The constructor can contain nested constructors. > > + > > + Note that a string literal rvalue can't be used to construct a > > char array. > > + It need one rvalue for each char. > > s/char array. It need one rvalue/char array; the latter needs one > rvalue/ > > > + > > + These entrypoints were added in :ref:`LIBGCCJIT_ABI_16`; you > > can test for its > > s/16/17/ I believe. > > s/its/their/ >
SV: [PATCH v2] jit: Add support for global rvalue initialization and ctors
Hi! > s/an union/a union/ > s/a rvalue/an rvalue/ Heh no way ... and I though I knew English grammar :) Had to look that up to see what the deal was but it makes sense. yunion, arevalue. > s/wrong-field-name/wrong-field-obj/ > > to match the struct example (given that the issue being tested for is > that it's the wrong object, rather than the wrong name). Initially, before submitting to the list, I made the code such that the field objects did not have to be the ones that were used when creating the struct or union, and forgot changing the test names. I figured it required too much string compares for the field names and pointer compares for the field object were more appropriate. To create dummy field objects were also kinda heavy. I'll address the points. Regards, Petter Från: David Malcolm Skickat: den 9 december 2021 20:39 Till: Petter Tomner; gcc-patches@gcc.gnu.org; j...@gcc.gnu.org; Antoni Boucher Ämne: Re: [PATCH v2] jit: Add support for global rvalue initialization and ctors On Mon, 2021-12-06 at 10:47 +, Petter Tomner via Gcc-patches wrote: > Hi! > > Attached is a patch with changes in line with the review of the prior > patch. > The patch adds support for initialization of global variables with > rvalues as well > as rvalue constructors for structs, arrays and unions. Thanks for the updated patch. Antoni: does this patch work for you for your rustc plugin? > > Review: https://gcc.gnu.org/pipermail/jit/2021q4/001400.html > > The points have been addressed, except: > > > Can the type be made const? > > I started to make types_kinda_same_internal () taking const args, but I > felt the > patch was ballooning because some spread out methods needed a const > signature too. I could submit that in a separate patch. Fair enough; fixing that isn't a blocker; it's already a big patch. > > I also addressed a problem Antoni found: > https://gcc.gnu.org/pipermail/jit/2021q4/001399.html > > , where you could not initialize global pointer variables to point to > uninitialized variables. I did that by > removing a redundant check with validate_var_has_init (), since that > walking function would > have to be quite complex to allow pointers to uninitialized variables. > > Any: > const int foo; > int bar = foo; > > will instead be reported as "not compile time constant" instead of a > nice error message with names. > > make check-jit runs fine on gnu-linux-x64 Debian. Various review comments inline below, which are mostly just nits: > From a4fef1308eaa72ce4ec51dbe5efcfbbf032e9870 Mon Sep 17 00:00:00 2001 > From: Petter Tomner > Date: Mon, 29 Nov 2021 20:44:07 +0100 > Subject: [PATCH] Add support for global rvalue initialization and constructors > > This patch adds support for initialization of global variables > with rvalues and creating constructors for array, struct and > union types which can be used as rvalues. > > Signed-off-by: > 2021-12-06 Petter Tomner [...snip...] > diff --git a/gcc/jit/docs/topics/expressions.rst > b/gcc/jit/docs/topics/expressions.rst > index 396259ef07e..5f64ca68595 100644 > --- a/gcc/jit/docs/topics/expressions.rst > +++ b/gcc/jit/docs/topics/expressions.rst > @@ -126,6 +126,147 @@ Simple expressions > underlying string, so it is valid to pass in a pointer to an on-stack > buffer. > > +Constructor expressions > +*** > + > + The following functions make constructors for array, struct and union > + types. > + > + The constructor rvalue can be used for assignment to locals. > + It can be used to initialize global variables with > + :func:`gcc_jit_global_set_initializer_rvalue`. It can also be used as a > + temporary value for function calls and return values, but its address > + can't be taken. > + > + Note that arrays in libgccjit does not collapse to pointers like in s/does not/do not/ > + C. I.e. if an array constructor is used as e.g. a return value, the whole > + array would be returned by value - array constructors can be assigned to > + array variables. > + > + The constructor can contain nested constructors. > + > + Note that a string literal rvalue can't be used to construct a char array. > + It need one rvalue for each char. s/char array. It need one rvalue/char array; the latter needs one rvalue/ > + > + These entrypoints were added in :ref:`LIBGCCJIT_ABI_16`; you can test for > its s/16/17/ I believe. s/its/their/ > + presense using: s/presense/presence/ (and in various other places below) > + .. code-block:: c > + #ifdef LIBGCCJIT_HAVE_CTORS > + > +.. function:: gcc_jit_rvalue *\ > + gcc_jit_context_new_array_constructor (gcc_jit_context *ctxt,\ > + gcc_jit_location *loc,\ > + gcc_jit_type *type,\ > + size_t arr_length,\ > + gcc_jit_r
