| Message ID | 1590327193-24688-1-git-send-email-kamleshbhalui@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2,PPC64,PR88877] | expand |
Hi! On Sun, May 24, 2020 at 07:03:13PM +0530, Kamlesh Kumar wrote: > In this patch series trying to address same by creating a struct Tuple > which bundles existing rtx and machine_mode and added one more > bool member which store unsigned_p which by default is false. The idea is good. However, you cannot call something as specific as this "tuple", in a header file that is used everywhere even. (We also do not have a "leading caps on types" convention). > This patch does not change underlying behavior yet. This will be done in > follow up patches. Thanks :-) > * rtl.h (Tuple): Defined and typedefed to rtx_mode_t. It's the other way around: rtx_mode_t is typedeffed to struct Tuple, so rtx_mode_t should be listed to the left of a : as well. OTOH, you don't need to name Tuple at all... It should not *have* a constructor, since you declared it as class... But you can just use std::tuple here? > (emit_library_call): Added default arg unsigned_p. > (emit_library_call_value): Added default arg unsigned_p. Yeah, eww. Default arguments have all the problems you had before, except now it is hidden and much more surprising. Those functions really should take rtx_mode_t arguments? Thanks again for working on this, Segher
> OTOH, you don't need to name Tuple at all... It should not *have* a > constructor, since you declared it as class... But you can just use > std::tuple here? I thought of using std::tuple but it requires c++11 support. I am not sure we always build gcc with c++11? > > > (emit_library_call): Added default arg unsigned_p. > > (emit_library_call_value): Added default arg unsigned_p. > > Yeah, eww. Default arguments have all the problems you had before, > except now it is hidden and much more surprising. > > Those functions really should take rtx_mode_t arguments? I was thinking the same. will incorporate this. ./kamlesh
On Mon, May 25, 2020 at 12:46:02PM +0530, kamlesh kumar wrote: > > OTOH, you don't need to name Tuple at all... It should not *have* a > > constructor, since you declared it as class... But you can just use > > std::tuple here? > > I thought of using std::tuple but it requires c++11 support. > I am not sure we always build gcc with c++11? https://gcc.gnu.org/install/prerequisites.html We do for GCC 11 :-) Since we pay the price for progress, let's reap the benefits as well :-) > > > (emit_library_call): Added default arg unsigned_p. > > > (emit_library_call_value): Added default arg unsigned_p. > > > > Yeah, eww. Default arguments have all the problems you had before, > > except now it is hidden and much more surprising. > > > > Those functions really should take rtx_mode_t arguments? > > I was thinking the same. will incorporate this. Thanks! Segher
On May 25, 2020 6:31:29 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote: >On Mon, May 25, 2020 at 12:46:02PM +0530, kamlesh kumar wrote: >> > OTOH, you don't need to name Tuple at all... It should not *have* >a >> > constructor, since you declared it as class... But you can just >use >> > std::tuple here? >> >> I thought of using std::tuple but it requires c++11 support. >> I am not sure we always build gcc with c++11? > >https://gcc.gnu.org/install/prerequisites.html > >We do for GCC 11 :-) Since we pay the price for progress, let's reap >the >benefits as well :-) Not sure if the benefit is enough to warrant an extra (complex?) standard header in almost every TU. >> > > (emit_library_call): Added default arg unsigned_p. >> > > (emit_library_call_value): Added default arg unsigned_p. >> > >> > Yeah, eww. Default arguments have all the problems you had before, >> > except now it is hidden and much more surprising. >> > >> > Those functions really should take rtx_mode_t arguments? >> >> I was thinking the same. will incorporate this. > >Thanks! > > >Segher
On Mon, May 25, 2020 at 06:37:57PM +0200, Richard Biener wrote: > >> I thought of using std::tuple but it requires c++11 support. > >> I am not sure we always build gcc with c++11? > > > >https://gcc.gnu.org/install/prerequisites.html > > > >We do for GCC 11 :-) Since we pay the price for progress, let's reap > >the > >benefits as well :-) > > Not sure if the benefit is enough to warrant an extra (complex?) standard header in almost every TU. Yeah, especially when one can just define the 3 fields of the small struct to be descriptive, rather than being first/second/third. Jakub
Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > On Mon, May 25, 2020 at 06:37:57PM +0200, Richard Biener wrote: >>>> I thought of using std::tuple but it requires c++11 support. >>>> I am not sure we always build gcc with c++11? >>> >>> https://gcc.gnu.org/install/prerequisites.html >>> >>> We do for GCC 11 :-) Since we pay the price for progress, let's reap >>> the >>> benefits as well :-) >> >> Not sure if the benefit is enough to warrant an extra (complex?) >> standard header in almost every TU. > > Yeah, especially when one can just define the 3 fields of the small struct > to be descriptive, rather than being first/second/third. +1, tuple (and pair) have the property that the use of the fields might be obvious to the code author, but probably will not be to the code reader. IMO most of the time as Jakub says something descriptive is better - and I don’t believe it’s a performance penalty. Iain
Thanks for working on this. Kamlesh Kumar via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > Here is a discussion we did some time ago regarding the defect. > https://gcc.gnu.org/pipermail/gcc/2019-January/227834.html > please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88877 for testcase > behavior. > > We incorporating below Jakub's suggestion in this patch series. > > Jakub wrote: > "" > Yeah, all the callers of emit_library_call* would need to be changed to pass > triplets rtx, machine_mode, int/bool /*unsignedp*/, instead of just > rtx_mode_t pair. > "" > > > In this patch series trying to address same by creating a struct Tuple > which bundles existing rtx and machine_mode and added one more > bool member which store unsigned_p which by default is false. > This patch does not change underlying behavior yet. This will be done in > follow up patches. > > ChangeLog Entry: > > 2020-05-24 Kamlesh Kumar <kamleshbhalui@gmail.com> > > * rtl.h (Tuple): Defined and typedefed to rtx_mode_t. > (emit_library_call): Added default arg unsigned_p. > (emit_library_call_value): Added default arg unsigned_p. > --- > gcc/rtl.h | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/gcc/rtl.h b/gcc/rtl.h > index b0b1aac..ee42de7 100644 > --- a/gcc/rtl.h > +++ b/gcc/rtl.h > @@ -2238,10 +2238,20 @@ struct address_info { > enum rtx_code base_outer_code; > }; > > -/* This is used to bundle an rtx and a mode together so that the pair > - can be used with the wi:: routines. If we ever put modes into rtx > - integer constants, this should go away and then just pass an rtx in. */ > -typedef std::pair <rtx, machine_mode> rtx_mode_t; > +/* This is used to bundle an rtx and a mode and unsignedness together so > + that the tuple can be used with the wi:: routines. If we ever put modes > + into rtx integer constants, this should go away and then just pass an rtx in. */ > +typedef struct Tuple { > + rtx first; > + machine_mode second; > + /* unsigned_p */ > + bool third; > + Tuple (rtx f, machine_mode s, bool t = false) { > + first = f; > + second = s; > + third = t; > + } > +} rtx_mode_t; > I think rtx_mode_t should continue to be what it is now: an rtx and a mode only. That's all that most rtl code cares about, since in rtl signedness is a property of an operation (where necessary) instead of an operand. (FWIW, the type was originally added to enable wide-int operators to be applied to rtx constants, to get around the fact that CONST_INT & co. don't store a mode. We should continue not to have signedness information in that situation.) I think instead we should make emit_library_call operate on a new and differently-named type that contains the three pieces of information. Thanks, Richard
On Sun, 2020-05-24 at 11:22 -0500, Segher Boessenkool wrote: > Hi! > > On Sun, May 24, 2020 at 07:03:13PM +0530, Kamlesh Kumar wrote: > > In this patch series trying to address same by creating a struct Tuple > > which bundles existing rtx and machine_mode and added one more > > bool member which store unsigned_p which by default is false. > > The idea is good. However, you cannot call something as specific as this > "tuple", in a header file that is used everywhere even. (We also do not > have a "leading caps on types" convention). > > > This patch does not change underlying behavior yet. This will be done in > > follow up patches. > > Thanks :-) > > > * rtl.h (Tuple): Defined and typedefed to rtx_mode_t. > > It's the other way around: rtx_mode_t is typedeffed to struct Tuple, so > rtx_mode_t should be listed to the left of a : as well. > > OTOH, you don't need to name Tuple at all... It should not *have* a > constructor, since you declared it as class... But you can just use > std::tuple here? > > > (emit_library_call): Added default arg unsigned_p. > > (emit_library_call_value): Added default arg unsigned_p. > > Yeah, eww. Default arguments have all the problems you had before, > except now it is hidden and much more surprising. > > Those functions really should take rtx_mode_t arguments? > > Thanks again for working on this, ISTM that using std::tuple would be better than defining our own types. I'd rather see the argument be explicit rather than using default arguments too. While I have ack'd some patches with default arguments, I still don't like 'em. I do like the approach of getting the infrastructure in place without changing behavior, then having the behavior fix as a distinct change. jeff
On Tue, Jun 09, 2020 at 02:29:13PM -0600, Jeff Law wrote: > On Sun, 2020-05-24 at 11:22 -0500, Segher Boessenkool wrote: > > OTOH, you don't need to name Tuple at all... It should not *have* a > > constructor, since you declared it as class... But you can just use > > std::tuple here? > > > > > (emit_library_call): Added default arg unsigned_p. > > > (emit_library_call_value): Added default arg unsigned_p. > > > > Yeah, eww. Default arguments have all the problems you had before, > > except now it is hidden and much more surprising. > > > > Those functions really should take rtx_mode_t arguments? > > > > Thanks again for working on this, > ISTM that using std::tuple would be better than defining our own types. Yeah. But as Jakub an Iain said, not using a container type (but a more concrete type, instead) is much better anyway :-) > I'd rather see the argument be explicit rather than using default arguments too. > While I have ack'd some patches with default arguments, I still don't like 'em. Default arguments have their place (but it's not here :-) ) > I do like the approach of getting the infrastructure in place without changing > behavior, then having the behavior fix as a distinct change. With Git, commits are easy and cheap, and massaging a patch series into shape is easy and cheap as well. If you develop using Git in the first place (and you should!), you should naturally end up with many patches in your series, and the preparatory patches first (after you reshuffle things a bit, if you are like me and your foresight is severly limited). So you have this separate *anyway* (or should have). Since it helps reviewing a lot, and also later bisecting, it is good to keep it. Segher
Thank you all for the suggestions.
This is first patch where I have just defined a struct libcall_arg_t
which contains
three member rtx, machine_mode and a boolean unsigned_p and will be
used in passing args in
emit_library_[call/value] functions.
Once this patch is approved then i will create second patch in which
arg type libcall_arg_t will be
propogated on all instances needed.
Then final patch which will fix the actual problem reported in PR88877.
ChangeLog Entry:
2020-06-13 Kamlesh Kumar <kamleshbhalui@gmail.com>
* rtl.h (libcall_arg_t): Defined.
---
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 0872cc4..c023ff0 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2238,6 +2238,18 @@ struct address_info {
enum rtx_code base_outer_code;
};
+/* This is used for passing args in emit_library_* functions */
+typedef struct libcall_arg {
+ rtx value;
+ machine_mode mode;
+ bool unsigned_p;
+ libcall_arg (rtx v, machine_mode m, bool u) {
+ value = v;
+ mode = m;
+ unsigned_p = u;
+ }
+} libcall_arg_t;
+
/* This is used to bundle an rtx and a mode together so that the pair
can be used with the wi:: routines. If we ever put modes into rtx
integer constants, this should go away and then just pass an rtx in. */
--
2.7.4
On Fri, Jun 12, 2020 at 4:43 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Tue, Jun 09, 2020 at 02:29:13PM -0600, Jeff Law wrote:
> > On Sun, 2020-05-24 at 11:22 -0500, Segher Boessenkool wrote:
> > > OTOH, you don't need to name Tuple at all... It should not *have* a
> > > constructor, since you declared it as class... But you can just use
> > > std::tuple here?
> > >
> > > > (emit_library_call): Added default arg unsigned_p.
> > > > (emit_library_call_value): Added default arg unsigned_p.
> > >
> > > Yeah, eww. Default arguments have all the problems you had before,
> > > except now it is hidden and much more surprising.
> > >
> > > Those functions really should take rtx_mode_t arguments?
> > >
> > > Thanks again for working on this,
> > ISTM that using std::tuple would be better than defining our own types.
>
> Yeah. But as Jakub an Iain said, not using a container type (but a more
> concrete type, instead) is much better anyway :-)
>
> > I'd rather see the argument be explicit rather than using default arguments too.
> > While I have ack'd some patches with default arguments, I still don't like 'em.
>
> Default arguments have their place (but it's not here :-) )
>
> > I do like the approach of getting the infrastructure in place without changing
> > behavior, then having the behavior fix as a distinct change.
>
> With Git, commits are easy and cheap, and massaging a patch series into
> shape is easy and cheap as well. If you develop using Git in the first
> place (and you should!), you should naturally end up with many patches
> in your series, and the preparatory patches first (after you reshuffle
> things a bit, if you are like me and your foresight is severly limited).
>
> So you have this separate *anyway* (or should have). Since it helps
> reviewing a lot, and also later bisecting, it is good to keep it.
>
>
> Segher
diff --git a/gcc/rtl.h b/gcc/rtl.h index b0b1aac..ee42de7 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -2238,10 +2238,20 @@ struct address_info { enum rtx_code base_outer_code; }; -/* This is used to bundle an rtx and a mode together so that the pair - can be used with the wi:: routines. If we ever put modes into rtx - integer constants, this should go away and then just pass an rtx in. */ -typedef std::pair <rtx, machine_mode> rtx_mode_t; +/* This is used to bundle an rtx and a mode and unsignedness together so + that the tuple can be used with the wi:: routines. If we ever put modes + into rtx integer constants, this should go away and then just pass an rtx in. */ +typedef struct Tuple { + rtx first; + machine_mode second; + /* unsigned_p */ + bool third; + Tuple (rtx f, machine_mode s, bool t = false) { + first = f; + second = s; + third = t; + } +} rtx_mode_t; namespace wi { @@ -4176,9 +4186,9 @@ emit_library_call (rtx fun, libcall_type fn_type, machine_mode outmode) inline void emit_library_call (rtx fun, libcall_type fn_type, machine_mode outmode, - rtx arg1, machine_mode arg1_mode) + rtx arg1, machine_mode arg1_mode, bool unsigned_p = false) { - rtx_mode_t args[] = { rtx_mode_t (arg1, arg1_mode) }; + rtx_mode_t args[] = { rtx_mode_t (arg1, arg1_mode, unsigned_p) }; emit_library_call_value_1 (0, fun, NULL_RTX, fn_type, outmode, 1, args); } @@ -4238,9 +4248,9 @@ emit_library_call_value (rtx fun, rtx value, libcall_type fn_type, inline rtx emit_library_call_value (rtx fun, rtx value, libcall_type fn_type, machine_mode outmode, - rtx arg1, machine_mode arg1_mode) + rtx arg1, machine_mode arg1_mode, bool unsigned_p = false) { - rtx_mode_t args[] = { rtx_mode_t (arg1, arg1_mode) }; + rtx_mode_t args[] = { rtx_mode_t (arg1, arg1_mode, unsigned_p) }; return emit_library_call_value_1 (1, fun, value, fn_type, outmode, 1, args); }
Here is a discussion we did some time ago regarding the defect. https://gcc.gnu.org/pipermail/gcc/2019-January/227834.html please see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88877 for testcase behavior. We incorporating below Jakub's suggestion in this patch series. Jakub wrote: "" Yeah, all the callers of emit_library_call* would need to be changed to pass triplets rtx, machine_mode, int/bool /*unsignedp*/, instead of just rtx_mode_t pair. "" In this patch series trying to address same by creating a struct Tuple which bundles existing rtx and machine_mode and added one more bool member which store unsigned_p which by default is false. This patch does not change underlying behavior yet. This will be done in follow up patches. ChangeLog Entry: 2020-05-24 Kamlesh Kumar <kamleshbhalui@gmail.com> * rtl.h (Tuple): Defined and typedefed to rtx_mode_t. (emit_library_call): Added default arg unsigned_p. (emit_library_call_value): Added default arg unsigned_p. --- gcc/rtl.h | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)