| Message ID | DDF8569C-14AF-4E64-AA91-0EA50A49E043@comcast.net |
|---|---|
| State | New |
| Headers | show |
On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: > Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. > > Ok? > > + *logical = wi::eq_p (t, 0) ? 0 : 1; I can't find the meaning of :: in n1256.pdf. What does this do? Also, given the complete lack of a description of what this patch does and no pointer to a discussion of what this patch does, and no description of its benefit to gfortran, I vote "no".
On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: > On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. >> >> Ok? >> >> + *logical = wi::eq_p (t, 0) ? 0 : 1; > > I can't find the meaning of :: in n1256.pdf. What does this do? wi:: eq_p means the function eq_p inside the wi struct. > > Also, given the complete lack of a description of what this > patch does and no pointer to a discussion of what this > patch does, and no description of its benefit to gfortran, > I vote "no". The general description was in a different email: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02877.html The main benefit is it allows for targets to support wider integer than two times HOST_WIDE_INT. So gfortran, is that it connects to the rest of the middle-end of GCC. Thanks, Andrew Pinski
On Sat, Nov 23, 2013 at 1:31 PM, Andrew Pinski <pinskia@gmail.com> wrote: > On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl > <sgk@troutmask.apl.washington.edu> wrote: >> On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >>> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. >>> >>> Ok? >>> >>> + *logical = wi::eq_p (t, 0) ? 0 : 1; >> >> I can't find the meaning of :: in n1256.pdf. What does this do? > > wi:: eq_p means the function eq_p inside the wi struct. > >> >> Also, given the complete lack of a description of what this >> patch does and no pointer to a discussion of what this >> patch does, and no description of its benefit to gfortran, >> I vote "no". > > The general description was in a different email: > http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02877.html > > The main benefit is it allows for targets to support wider integer > than two times HOST_WIDE_INT. So gfortran, is that it connects to the > rest of the middle-end of GCC. One more comment here, all of the changes in the gfortran front-end is the trans* functions which is the interface between the front-end and GCC middle-end. > > > Thanks, > Andrew Pinski
On Sat, Nov 23, 2013 at 01:31:04PM -0800, Andrew Pinski wrote: > On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl > <sgk@troutmask.apl.washington.edu> wrote: > > On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: > >> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. > >> > >> Ok? > >> > >> + *logical = wi::eq_p (t, 0) ? 0 : 1; > > > > I can't find the meaning of :: in n1256.pdf. What does this do? > > wi:: eq_p means the function eq_p inside the wi struct. Where in n1256.pdf can I read about this feature? > > > > Also, given the complete lack of a description of what this > > patch does and no pointer to a discussion of what this > > patch does, and no description of its benefit to gfortran, > > I vote "no". > > The general description was in a different email: > http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02877.html Should have been included in the initial email.
On Sat, Nov 23, 2013 at 4:26 PM, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: > On Sat, Nov 23, 2013 at 01:31:04PM -0800, Andrew Pinski wrote: >> On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl >> <sgk@troutmask.apl.washington.edu> wrote: >> > On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >> >> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. >> >> >> >> Ok? >> >> >> >> + *logical = wi::eq_p (t, 0) ? 0 : 1; >> > >> > I can't find the meaning of :: in n1256.pdf. What does this do? >> >> wi:: eq_p means the function eq_p inside the wi struct. > > Where in n1256.pdf can I read about this feature? This is a C++ feature and not a C feature. Thanks, Andrew > >> > >> > Also, given the complete lack of a description of what this >> > patch does and no pointer to a discussion of what this >> > patch does, and no description of its benefit to gfortran, >> > I vote "no". >> >> The general description was in a different email: >> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02877.html > > Should have been included in the initial email. > > -- > Steve
Steve Kargl <sgk@troutmask.apl.washington.edu> writes: > On Sat, Nov 23, 2013 at 01:31:04PM -0800, Andrew Pinski wrote: >> On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl >> <sgk@troutmask.apl.washington.edu> wrote: >> > On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >> >> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. >> >> >> >> Ok? >> >> >> >> + *logical = wi::eq_p (t, 0) ? 0 : 1; >> > >> > I can't find the meaning of :: in n1256.pdf. What does this do? >> >> wi:: eq_p means the function eq_p inside the wi struct. > > Where in n1256.pdf can I read about this feature? Try n3376.pdf instead. Andreas.
On Nov 23 2013, Andrew Pinski wrote: >On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl ><sgk@troutmask.apl.washington.edu> wrote: >> On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >>> >>> Richi has asked the we break the wide-int patch so that the individual >>> port and front end maintainers can review their parts without have to >>> go through the entire patch. This patch covers the fortran front end. >>> >>> Ok? >>> >>> + *logical = wi::eq_p (t, 0) ? 0 : 1; >> >> I can't find the meaning of :: in n1256.pdf. What does this do? > >wi:: eq_p means the function eq_p inside the wi struct. But you can't tell that from the code - wi might be a namespace, and eq_p might be a class. If there is a clear description of the subset of C++ that the front-end is allowed to use, a pointer to it for the benefit of Fortran/C/Ada/whatever people would be useful. But that's an aside from this thread. >> Also, given the complete lack of a description of what this >> patch does and no pointer to a discussion of what this >> patch does, and no description of its benefit to gfortran, >> I vote "no". > >The general description was in a different email: >http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02877.html > >The main benefit is it allows for targets to support wider integer >than two times HOST_WIDE_INT. So gfortran, is that it connects to the >rest of the middle-end of GCC. Hmm. Looking at that makes me none the wiser, and even a web search doesn't do more than tell me the same aspects. Given that Fortran has somewhat different constraints on type widths than C, it would be very useful to know exactly what you mean by that. C++ is almost entirely irrelevant here. Now, obviously to an implementor, it doesn't mean unlimited-width types, but does it (a) allow implementations to use hardware/firmware/emulated built-in types however wide they are, (b) allow arbitrary-width types, or (c) something else? In all cases, I can see implementation issues in gfortran that do not arise in gcc, but they are different between those models. So I agree that some clarification would be a good idea, to avoid introducing secondary problems by accident. Quite likely there will be none, but it's hard to tell. Regards, Nick Maclaren.
Mike Stump wrote:
> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end.
Nice clean up. The new class looks much cleaner as it avoids the LO/HI
handling.
- hi = TREE_INT_CST_HIGH (bound);
- low = TREE_INT_CST_LOW (bound);
- if (hi || low < 0
- || ((!as || as->type != AS_ASSUMED_RANK)
- && low >= GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc)))
- || low > GFC_MAX_DIMENSIONS)
+ if (((!as || as->type != AS_ASSUMED_RANK)
+ && wi::geu_p (bound, GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc))))
+ || wi::gtu_p (bound, GFC_MAX_DIMENSIONS))
gfc_error ("'dim' argument of %s intrinsic at %L is not a valid "
"dimension index", upper ? "UBOUND" : "LBOUND",
&expr->where);
I don't see what happened to the "low < 0" check. (Ditto for the next chunk in conv_intrinsic_cobound).
Otherwise, it looks okay to me.
Tobias
On 11/24/2013 05:16 AM, N.M. Maclaren wrote: > On Nov 23 2013, Andrew Pinski wrote: >> On Sat, Nov 23, 2013 at 12:16 PM, Steve Kargl >> <sgk@troutmask.apl.washington.edu> wrote: >>> On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >>>> >>>> Richi has asked the we break the wide-int patch so that the >>>> individual port and front end maintainers can review their parts >>>> without have to go through the entire patch. This patch covers the >>>> fortran front end. >>>> >>>> Ok? >>>> >>>> + *logical = wi::eq_p (t, 0) ? 0 : 1; >>> >>> I can't find the meaning of :: in n1256.pdf. What does this do? >> >> wi:: eq_p means the function eq_p inside the wi struct. > > But you can't tell that from the code - wi might be a namespace, and > eq_p might be a class. If there is a clear description of the subset > of C++ that the front-end is allowed to use, a pointer to it for the > benefit of Fortran/C/Ada/whatever people would be useful. But that's > an aside from this thread. There is a saying in the US that "you can always tell the pioneers because they are the ones with the arrows in their back." I feel this way with respect to C++. When this branch first went public several months ago, the wide-int class made very modest use of C++. It has publicly evolved away from that and i would now describe the usage as somewhat aggressive. I do not know if i think that this is good or bad, but i expect that because we are the first big patch to use C++ aggressively , that we are going to take some arrows. > >>> Also, given the complete lack of a description of what this >>> patch does and no pointer to a discussion of what this >>> patch does, and no description of its benefit to gfortran, >>> I vote "no". >> >> The general description was in a different email: >> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02877.html >> >> The main benefit is it allows for targets to support wider integer >> than two times HOST_WIDE_INT. So gfortran, is that it connects to the >> rest of the middle-end of GCC. > > Hmm. Looking at that makes me none the wiser, and even a web search > doesn't do more than tell me the same aspects. Given that Fortran has > somewhat different constraints on type widths than C, it would be very > useful to know exactly what you mean by that. C++ is almost entirely > irrelevant here. > A useful way to think about this patch is that it is nothing but plumbing. Gcc has had a painful and long evolution from being a 32 bit compiler to a 64 bit compiler and beyond. The primary goal of this patch is change the underlying data structures used to represent integer constants from being either single host wide ints (that were limited to 64 bits) and double-int (that worked reliably for 127 bits and mostly ok for 128 bits) to a data structure that would just work reliably for any precision integer that a back end supported. But at both ends of the compiler there are still limits. It is expected that with this patch in place, that a back end maintainer can now, for the first time, support architectures that support integers wider than 128 bits. We have several public ports that appear to have tried to support some operations on 256 bits but when we tried this on our private port, we found an intolerable number of ices and incorrect code issues. They are now gone. To the extent that this patch touches the front ends, this patch is strictly plumbing. The front ends now use the new wide-int class, or the new int-cst to pass integer constants around and do constant prop on them. Beyond that, it is up to the front end maintainers to decide how or even if they will expose this to the users of the language. C and C++ currently allow the back end to tell it that they can use wider types but actually specifying a large integer constant is at best painful. > Now, obviously to an implementor, it doesn't mean unlimited-width types, > but does it (a) allow implementations to use hardware/firmware/emulated > built-in types however wide they are, (b) allow arbitrary-width types, > or (c) something else? In all cases, I can see implementation issues > in gfortran that do not arise in gcc, but they are different between > those models. the patch sniffs the port (in particular, it looks at the extra modes specified by the port), and allows integers that are some multiple of size of the largest mode specified by the back end. In this way ports for small machines pay less of price than the big ports will. One could imagine that this is not good enough if a front end wanted to say that it supports everything up to 256 bits no matter what the hardware can do natively, much the way that java said that every host had to do 64 bit math, even if it was just a 32 bit host. However, that would require a lot more work than was done here. In particular, at some level someone would have to sniff the port and break the math into pieces that could be implemented on the port. > > So I agree that some clarification would be a good idea, to avoid > introducing secondary problems by accident. Quite likely there will be > none, but it's hard to tell. > > > Regards, > Nick Maclaren. > > >
On 11/24/2013 05:50 AM, Tobias Burnus wrote: > Mike Stump wrote: >> Richi has asked the we break the wide-int patch so that the >> individual port and front end maintainers can review their parts >> without have to go through the entire patch. This patch covers the >> fortran front end. > > Nice clean up. The new class looks much cleaner as it avoids the LO/HI > handling. > > > - hi = TREE_INT_CST_HIGH (bound); > - low = TREE_INT_CST_LOW (bound); > - if (hi || low < 0 > - || ((!as || as->type != AS_ASSUMED_RANK) > - && low >= GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc))) > - || low > GFC_MAX_DIMENSIONS) > + if (((!as || as->type != AS_ASSUMED_RANK) > + && wi::geu_p (bound, GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc)))) > + || wi::gtu_p (bound, GFC_MAX_DIMENSIONS)) > gfc_error ("'dim' argument of %s intrinsic at %L is not a valid " > "dimension index", upper ? "UBOUND" : "LBOUND", > &expr->where); > > I don't see what happened to the "low < 0" check. (Ditto for the next > chunk in conv_intrinsic_cobound). > > Otherwise, it looks okay to me. > > Tobias > This is the magic of using the correct representation. All this code really wanted to check is that bound is a small positive integer.
On Nov 24 2013, Kenneth Zadeck wrote: Thank you for your posting. That certainly clears up my understanding. >> If there is a clear description of the subset >> of C++ that the front-end is allowed to use, a pointer to it for the >> benefit of Fortran/C/Ada/whatever people would be useful. But that's >> an aside from this thread. > >There is a saying in the US that "you can always tell the pioneers >because they are the ones with the arrows in their back." I feel this >way with respect to C++. When this branch first went public several >months ago, the wide-int class made very modest use of C++. It has >publicly evolved away from that and i would now describe the usage as >somewhat aggressive. I do not know if i think that this is good or >bad, but i expect that because we are the first big patch to use C++ >aggressively , that we are going to take some arrows. I think that using C++ even slightly aggressively without first deciding the grounds rules is a seriously bad idea. The trouble about following random pioneers is that they are very likely to lead you into a swamp, with the loss of the whole expedition. And, with C++, that is more than just likely if you follow an adventurous pioneer rather than a cautious one - it's damn-near certain :-( >A useful way to think about this patch is that it is nothing but >plumbing. Gcc has had a painful and long evolution from being a 32 >bit compiler to a 64 bit compiler and beyond. ... Yes, but that's not my point. The main problem is that integer constant expressions in C are limited to the built-in operators, of which the only tricky ones are division and remainder (and, occasionally, multiplication) - see C11 6.6#3. Fortran is not so limited, and there are much wider requirements for expression evaluation at compile time. >But at both ends of the compiler there are still limits. And my point is that this is NOT an area that separates cleanly into front and back end! However, from your description, this is one component of any solution for gfortran, and it doesn't sound as if it will cause trouble until and unless someone wants to extend gfortran to wider types. They will then have to implement the other components .... Regards, Nick Maclaren.
Hi, On 2013-11-24 22:38, N.M. Maclaren wrote: > The main problem is that integer constant expressions in C are limited to > the built-in operators, of which the only tricky ones are division and > remainder (and, occasionally, multiplication) - see C11 6.6#3. Fortran > is not so limited, and there are much wider requirements for expression > evaluation at compile time. please note that the standard-mandated compile-time evaluation is handled almost [*] entirely inside the Fortran frontend, viz. arith.c, resolve.c, simplify.c, intrinsic.c, maybe others, whereas the wide-int changes deal with the middle-end interface and touches none of these files. Cheers, - Tobi [*] TRANSFER the a special case because it needs to know memory layouts >> But at both ends of the compiler there are still limits. > > And my point is that this is NOT an area that separates cleanly into > front and back end! > > However, from your description, this is one component of any solution > for gfortran, and it doesn't sound as if it will cause trouble until > and unless someone wants to extend gfortran to wider types. They will > then have to implement the other components .... > > > Regards, > Nick Maclaren. > > >
Am 24.11.2013 13:56, schrieb Kenneth Zadeck: > > On 11/24/2013 05:50 AM, Tobias Burnus wrote: >> - if (hi || low < 0 >> ... >> + if (((!as || as->type != AS_ASSUMED_RANK) >> + && wi::geu_p (bound, GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc)))) >> + || wi::gtu_p (bound, GFC_MAX_DIMENSIONS)) >> gfc_error ("'dim' argument of %s intrinsic at %L is not a valid " >> "dimension index", upper ? "UBOUND" : "LBOUND", >> &expr->where); >> >> I don't see what happened to the "low < 0" check. (Ditto for the next >> chunk in conv_intrinsic_cobound). >> Otherwise, it looks okay to me. >> > This is the magic of using the correct representation. All this code > really wanted to check is that bound is a small positive integer. I think one of the problems I had with reviewing the patch – as others – that there is no documentation for the functions provided by the patch itself. I found the bits in the patch wide-int-wide.diffs.txt.bz2. (The .bz2 successfully prevents that it is found by search engines, but admittedly, I haven't tried to find it.) Nice – albeit a bit intransparent – use of two's complement numbers and casting to an unsigned number. +/* Return true if X > Y when both are treated as unsigned values. */ +template <typename T1, typename T2> +inline bool +wi::gtu_p (const T1 &x, const T2 &y) +{ + return ltu_p (y, x); Thus, the patch is okay from my side. * * * >>> + *logical = wi::eq_p (t, 0) ? 0 : 1; >> >> I can't find the meaning of :: in n1256.pdf. What does this do? > wi:: eq_p means the function eq_p inside the wi struct. I have to admit that I didn't quite understand the discussion about ISO C/ISO C++11 as my impression is that wide-int-wide.diffs.txt.bz2 and not the ISO standards matter here. Also the discussion about unlimited-width types seems to be a bit off as it is "only" about the use within the compiler (when the host lacks an integer type the target has) and not about support for compiled code on the target. But maybe I have missed some fine print. > But at both ends of the compiler there are still limits. It is > expected that with this patch in place, that a back end maintainer can > now, for the first time, support architectures that support integers > wider than 128 bits. We have several public ports that appear to have > tried to support some operations on 256 bits but when we tried this on > our private port, we found an intolerable number of ices and incorrect > code issues. They are now gone. > > To the extent that this patch touches the front ends, this patch is > strictly plumbing. The front ends now use the new wide-int class, or > the new int-cst to pass integer constants around and do constant prop > on them. Beyond that, it is up to the front end maintainers to decide > how or even if they will expose this to the users of the language. C > and C++ currently allow the back end to tell it that they can use > wider types but actually specifying a large integer constant is at > best painful. I think the gfortran code currently implicitly assumes that the largest integer is 128bits wide; adding basic support for larger integer kinds is probably not difficult. However, the real work is to fully support it with all intrinsics. Already for 128bit integers, some work is needed as there for some of the intrinsics, a suitable builtin is lacking. (I think gfortran is the only Fortran compiler which supports 128-bits/16-byte integers; at least all compilers I tried do not support it.) – I think when GCC starts to get some better support for 256-bit integers, gfortran will follow suite. (Or when someone explicitly demands it.) > One could imagine that this is not good enough if a front end wanted > to say that it supports everything up to 256 bits no matter what the > hardware can do natively, much the way that java said that every host > had to do 64 bit math, even if it was just a 32 bit host. However, > that would require a lot more work than was done here. In particular, > at some level someone would have to sniff the port and break the math > into pieces that could be implemented on the port. In particular, that requires some run-time support – probably both in libgcc and in the handling of the backend itself. On x86-64, __float128 support already works that way. (And requires libquadmath to be fully usable; for integers, it should be somewhat easier.) Tobias
On Sun, Nov 24, 2013 at 2:38 PM, N.M. Maclaren wrote: > The main problem is that integer constant expressions in C are limited to > the built-in operators, of which the only tricky ones are division and > remainder (and, occasionally, multiplication) - see C11 6.6#3. Fortran > is not so limited, and there are much wider requirements for expression > evaluation at compile time. In gfortran there's been effectively no limit on the size of integers since the beginning, because all integers are represented as mpz (i.e. GMP integers) values. In fact, I'm a bit surprised that the wide_int stuff isn't also based on GMP's integer representation, but oh well... Point is, this wide_int stuff is for the interface between gfortran as a front end on one side, and the rest of gcc (middle end) on the other. At the hand-over point, gfortran will already have handled more complex integer constant expressions as much as possible. Ciao! Steven
On 11/24/2013 08:38 AM, N.M. Maclaren wrote: > On Nov 24 2013, Kenneth Zadeck wrote: > > Thank you for your posting. That certainly clears up my understanding. > >>> If there is a clear description of the subset >>> of C++ that the front-end is allowed to use, a pointer to it for the >>> benefit of Fortran/C/Ada/whatever people would be useful. But that's >>> an aside from this thread. >> >> There is a saying in the US that "you can always tell the pioneers >> because they are the ones with the arrows in their back." I feel >> this way with respect to C++. When this branch first went public >> several months ago, the wide-int class made very modest use of C++. >> It has publicly evolved away from that and i would now describe the >> usage as somewhat aggressive. I do not know if i think that this >> is good or bad, but i expect that because we are the first big patch >> to use C++ aggressively , that we are going to take some arrows. > > I think that using C++ even slightly aggressively without first deciding > the grounds rules is a seriously bad idea. The trouble about following > random pioneers is that they are very likely to lead you into a swamp, > with the loss of the whole expedition. And, with C++, that is more than > just likely if you follow an adventurous pioneer rather than a cautious > one - it's damn-near certain :-( > i do not actually disagree with you. my point is that this was all done in the open. >> A useful way to think about this patch is that it is nothing but >> plumbing. Gcc has had a painful and long evolution from being a 32 >> bit compiler to a 64 bit compiler and beyond. ... > > Yes, but that's not my point. > > The main problem is that integer constant expressions in C are limited to > the built-in operators, of which the only tricky ones are division and > remainder (and, occasionally, multiplication) - see C11 6.6#3. Fortran > is not so limited, and there are much wider requirements for expression > evaluation at compile time. > >> But at both ends of the compiler there are still limits. > > And my point is that this is NOT an area that separates cleanly into > front and back end! > > However, from your description, this is one component of any solution > for gfortran, and it doesn't sound as if it will cause trouble until > and unless someone wants to extend gfortran to wider types. They will > then have to implement the other components .... > > In any event, the problem is not with making the math work correctly, which is really the primary point of this patch. It is as you say how the various front ends decide how they wish to exploit that power. > Regards, > Nick Maclaren. > > >
On Nov 23, 2013, at 12:16 PM, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: > On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >> Richi has asked the we break the wide-int patch so that the individual port and front end maintainers can review their parts without have to go through the entire patch. This patch covers the fortran front end. >> >> Ok? >> >> + *logical = wi::eq_p (t, 0) ? 0 : 1; > > I can't find the meaning of :: in n1256.pdf. What does this do? > > Also, given the complete lack of a description of what this > patch does and no pointer to a discussion of what this > patch does, and no description of its benefit to gfortran, > I vote "no". I don't like the notion that one person says yes, and one says no, and then we ignore the no, and use the yes to approve a patch. Can the fortran come up with a final unified answer… Thanks. The quoted code above merely tests to see if t is equal to 0, and set *logical to 0 is it is 0 and 1 if it's not. wi::eq_p is read in English as, does equality hold in the wide-int sense of equality? mpz::eq_p would mean then, does equality hold in the mpz sense of equality? tree::eq_p would mean then, does equality hold in the tree sense of equality? If you interested in the lower layer answer, wi::eq_p() calls a function, whose name is wi::eq_p, with the given arguments, just like foo() calls foo, with the given arguments. operator +(1, 2), for example, calls a function whose name is operator +, with the two arguments 1 and 2. In C++ names, if you will, can be arbitrarily composed and grouped using types (classes) and namespaces. classes are composition of 0 or more other types (think struct in C), and namespaces are not. wi, is such a namespace and relates to a concept called wide int, inside that concept, there is a concept of equality; however, there is no data with wi, it isn't a class (a struct). In normal C, it would be spelled wi_eq_p () as C doesn't (yet) have a grouping feature. As for the patch, it is a simple bug fix to fix a few ICEs and wrong code generation bugs present in gcc. Without it, gcc ICEs and produces wrong code, with it, it doesn't. The bugs are usually in the 2*HWI and larger sizes (64 bits for 32 hosts and 128 bit for 64 bit hosts). The benefit to fortran would be support for more correct code gen and a lessoning of ICEs on valid code. Ok?
On Wed, Jan 01, 2014 at 07:49:16PM -0800, Mike Stump wrote: > On Nov 23, 2013, at 12:16 PM, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: >> On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >>> Richi has asked the we break the wide-int patch so that the >>> individual port and front end maintainers can review their >>> parts without have to go through the entire patch. This >>> patch covers the fortran front end. >>> >>> Ok? >>> >>> + *logical = wi::eq_p (t, 0) ? 0 : 1; >> >> I can't find the meaning of :: in n1256.pdf. What does this do? >> >> Also, given the complete lack of a description of what this >> patch does and no pointer to a discussion of what this >> patch does, and no description of its benefit to gfortran, >> I vote "no". > > I don't like the notion that one person says yes, and one says no, > and then we ignore the no, and use the yes to approve a patch. Can > the fortran come up with a final unified answer? Thanks. My original comment had nothing to do with the technical merit of the patch. My comment, as shown above, objected to dropping a patch on (gfortran) developers with *no* explanation of the patch and *no* pointer to where a discussion may have occurred.
On Sat, Jan 4, 2014 at 9:13 PM, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: > On Wed, Jan 01, 2014 at 07:49:16PM -0800, Mike Stump wrote: >> On Nov 23, 2013, at 12:16 PM, Steve Kargl <sgk@troutmask.apl.washington.edu> wrote: >>> On Sat, Nov 23, 2013 at 11:21:21AM -0800, Mike Stump wrote: >>>> Richi has asked the we break the wide-int patch so that the >>>> individual port and front end maintainers can review their >>>> parts without have to go through the entire patch. This >>>> patch covers the fortran front end. >>>> >>>> Ok? >>>> >>>> + *logical = wi::eq_p (t, 0) ? 0 : 1; >>> >>> I can't find the meaning of :: in n1256.pdf. What does this do? >>> >>> Also, given the complete lack of a description of what this >>> patch does and no pointer to a discussion of what this >>> patch does, and no description of its benefit to gfortran, >>> I vote "no". >> >> I don't like the notion that one person says yes, and one says no, >> and then we ignore the no, and use the yes to approve a patch. Can >> the fortran come up with a final unified answer? Thanks. > > My original comment had nothing to do with the technical merit > of the patch. My comment, as shown above, objected to dropping > a patch on (gfortran) developers with *no* explanation of the > patch and *no* pointer to where a discussion may have occurred. Note that at least some of the patch, like @@ -430,7 +431,7 @@ gfc_interpret_logical (int kind, unsigned char *buffer, size_t buffer_size, { tree t = native_interpret_expr (gfc_get_logical_type (kind), buffer, buffer_size); - *logical = tree_to_double_int (t).is_zero () ? 0 : 1; + *logical = wi::eq_p (t, 0) ? 0 : 1; return size_logical (kind); } could have used a proper tree interface instead. In the above case simply *logical = integer_zerop (t) ? 0 : 1; @@ -2083,13 +2083,14 @@ gfc_conv_cst_int_power (gfc_se * se, tree lhs, tree rhs) HOST_WIDE_INT m; unsigned HOST_WIDE_INT n; int sgn; + wide_int wrhs = rhs; /* If exponent is too large, we won't expand it anyway, so don't bother with large integer values. */ - if (!TREE_INT_CST (rhs).fits_shwi ()) + if (!wi::fits_shwi_p (wrhs)) return 0; similar, tree_fits_swhi_p (). Not sure if that helps wrt acceptance. Richard. > -- > Steve
diff --git a/gcc/fortran/target-memory.c b/gcc/fortran/target-memory.c index d0ee41a..8778d98 100644 --- a/gcc/fortran/target-memory.c +++ b/gcc/fortran/target-memory.c @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3. If not see #include "trans-const.h" #include "trans-types.h" #include "target-memory.h" +#include "wide-int.h" /* --------------------------------------------------------------- */ /* Calculate the size of an expression. */ @@ -430,7 +431,7 @@ gfc_interpret_logical (int kind, unsigned char *buffer, size_t buffer_size, { tree t = native_interpret_expr (gfc_get_logical_type (kind), buffer, buffer_size); - *logical = tree_to_double_int (t).is_zero () ? 0 : 1; + *logical = wi::eq_p (t, 0) ? 0 : 1; return size_logical (kind); } diff --git a/gcc/fortran/trans-array.c b/gcc/fortran/trans-array.c index c2bbd0e..5b3cd1f 100644 --- a/gcc/fortran/trans-array.c +++ b/gcc/fortran/trans-array.c @@ -90,6 +90,7 @@ along with GCC; see the file COPYING3. If not see #include "trans-array.h" #include "trans-const.h" #include "dependency.h" +#include "wide-int.h" static bool gfc_get_array_constructor_size (mpz_t *, gfc_constructor_base); @@ -5361,9 +5362,8 @@ gfc_conv_array_initializer (tree type, gfc_expr * expr) { gfc_constructor *c; tree tmp; + offset_int wtmp; gfc_se se; - HOST_WIDE_INT hi; - unsigned HOST_WIDE_INT lo; tree index, range; vec<constructor_elt, va_gc> *v = NULL; @@ -5385,20 +5385,13 @@ gfc_conv_array_initializer (tree type, gfc_expr * expr) else gfc_conv_structure (&se, expr, 1); - tmp = TYPE_MAX_VALUE (TYPE_DOMAIN (type)); - gcc_assert (tmp && INTEGER_CST_P (tmp)); - hi = TREE_INT_CST_HIGH (tmp); - lo = TREE_INT_CST_LOW (tmp); - lo++; - if (lo == 0) - hi++; + wtmp = wi::to_offset (TYPE_MAX_VALUE (TYPE_DOMAIN (type))) + 1; + gcc_assert (wtmp != 0); /* This will probably eat buckets of memory for large arrays. */ - while (hi != 0 || lo != 0) + while (wtmp != 0) { CONSTRUCTOR_APPEND_ELT (v, NULL_TREE, se.expr); - if (lo == 0) - hi--; - lo--; + wtmp -= 1; } break; diff --git a/gcc/fortran/trans-const.c b/gcc/fortran/trans-const.c index f5a2b18..334b5f5 100644 --- a/gcc/fortran/trans-const.c +++ b/gcc/fortran/trans-const.c @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3. If not see #include "trans-const.h" #include "trans-types.h" #include "target-memory.h" +#include "wide-int.h" tree gfc_rank_cst[GFC_MAX_DIMENSIONS + 1]; @@ -145,8 +146,7 @@ gfc_conv_string_init (tree length, gfc_expr * expr) gcc_assert (expr->expr_type == EXPR_CONSTANT); gcc_assert (expr->ts.type == BT_CHARACTER); - gcc_assert (INTEGER_CST_P (length)); - gcc_assert (TREE_INT_CST_HIGH (length) == 0); + gcc_assert (tree_fits_uhwi_p (length)); len = TREE_INT_CST_LOW (length); slen = expr->value.character.length; @@ -201,8 +201,8 @@ gfc_init_constants (void) tree gfc_conv_mpz_to_tree (mpz_t i, int kind) { - double_int val = mpz_get_double_int (gfc_get_int_type (kind), i, true); - return double_int_to_tree (gfc_get_int_type (kind), val); + wide_int val = wi::from_mpz (gfc_get_int_type (kind), i, true); + return wide_int_to_tree (gfc_get_int_type (kind), val); } /* Converts a backend tree into a GMP integer. */ @@ -210,8 +210,7 @@ gfc_conv_mpz_to_tree (mpz_t i, int kind) void gfc_conv_tree_to_mpz (mpz_t i, tree source) { - double_int val = tree_to_double_int (source); - mpz_set_double_int (i, val, TYPE_UNSIGNED (TREE_TYPE (source))); + wi::to_mpz (source, i, TYPE_SIGN (TREE_TYPE (source))); } /* Converts a real constant into backend form. */ diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c index f974c6e..6a29ba1 100644 --- a/gcc/fortran/trans-decl.c +++ b/gcc/fortran/trans-decl.c @@ -409,7 +409,7 @@ gfc_can_put_var_on_stack (tree size) if (gfc_option.flag_max_stack_var_size < 0) return 1; - if (TREE_INT_CST_HIGH (size) != 0) + if (!tree_fits_uhwi_p (size)) return 0; low = TREE_INT_CST_LOW (size); diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c index 41b2f94..bb5cfad 100644 --- a/gcc/fortran/trans-expr.c +++ b/gcc/fortran/trans-expr.c @@ -41,7 +41,7 @@ along with GCC; see the file COPYING3. If not see #include "dependency.h" #include "gimple.h" #include "gimplify.h" - +#include "wide-int.h" /* Convert a scalar to an array descriptor. To be used for assumed-rank arrays. */ @@ -2083,13 +2083,14 @@ gfc_conv_cst_int_power (gfc_se * se, tree lhs, tree rhs) HOST_WIDE_INT m; unsigned HOST_WIDE_INT n; int sgn; + wide_int wrhs = rhs; /* If exponent is too large, we won't expand it anyway, so don't bother with large integer values. */ - if (!TREE_INT_CST (rhs).fits_shwi ()) + if (!wi::fits_shwi_p (wrhs)) return 0; - m = TREE_INT_CST (rhs).to_shwi (); + m = wrhs.to_shwi (); /* There's no ABS for HOST_WIDE_INT, so here we go. It also takes care of the asymmetric range of the integer type. */ n = (unsigned HOST_WIDE_INT) (m < 0 ? -m : m); @@ -2628,7 +2629,7 @@ gfc_string_to_single_character (tree len, tree str, int kind) { if (len == NULL - || !INTEGER_CST_P (len) || TREE_INT_CST_HIGH (len) != 0 + || !tree_fits_uhwi_p (len) || !POINTER_TYPE_P (TREE_TYPE (str))) return NULL_TREE; @@ -2742,8 +2743,9 @@ gfc_optimize_len_trim (tree len, tree str, int kind) && TREE_CODE (TREE_OPERAND (TREE_OPERAND (str, 0), 0)) == STRING_CST && array_ref_low_bound (TREE_OPERAND (str, 0)) == TREE_OPERAND (TREE_OPERAND (str, 0), 1) - && TREE_INT_CST_LOW (len) >= 1 - && TREE_INT_CST_LOW (len) + && tree_fits_uhwi_p (len) + && tree_to_uhwi (len) >= 1 + && tree_to_uhwi (len) == (unsigned HOST_WIDE_INT) TREE_STRING_LENGTH (TREE_OPERAND (TREE_OPERAND (str, 0), 0))) { diff --git a/gcc/fortran/trans-intrinsic.c b/gcc/fortran/trans-intrinsic.c index 4acdc8d..7e5feab 100644 --- a/gcc/fortran/trans-intrinsic.c +++ b/gcc/fortran/trans-intrinsic.c @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. If not see /* Only for gfc_trans_assign and gfc_trans_pointer_assign. */ #include "trans-stmt.h" #include "tree-nested.h" +#include "wide-int.h" /* This maps Fortran intrinsic math functions to external library or GCC builtin functions. */ @@ -987,12 +988,8 @@ trans_this_image (gfc_se * se, gfc_expr *expr) if (INTEGER_CST_P (dim_arg)) { - int hi, co_dim; - - hi = TREE_INT_CST_HIGH (dim_arg); - co_dim = TREE_INT_CST_LOW (dim_arg); - if (hi || co_dim < 1 - || co_dim > GFC_TYPE_ARRAY_CORANK (TREE_TYPE (desc))) + if (wi::ltu_p (dim_arg, 1) + || wi::gtu_p (dim_arg, GFC_TYPE_ARRAY_CORANK (TREE_TYPE (desc)))) gfc_error ("'dim' argument of %s intrinsic at %L is not a valid " "dimension index", expr->value.function.isym->name, &expr->where); @@ -1349,14 +1346,9 @@ gfc_conv_intrinsic_bound (gfc_se * se, gfc_expr * expr, int upper) if (INTEGER_CST_P (bound)) { - int hi, low; - - hi = TREE_INT_CST_HIGH (bound); - low = TREE_INT_CST_LOW (bound); - if (hi || low < 0 - || ((!as || as->type != AS_ASSUMED_RANK) - && low >= GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc))) - || low > GFC_MAX_DIMENSIONS) + if (((!as || as->type != AS_ASSUMED_RANK) + && wi::geu_p (bound, GFC_TYPE_ARRAY_RANK (TREE_TYPE (desc)))) + || wi::gtu_p (bound, GFC_MAX_DIMENSIONS)) gfc_error ("'dim' argument of %s intrinsic at %L is not a valid " "dimension index", upper ? "UBOUND" : "LBOUND", &expr->where); @@ -1551,11 +1543,8 @@ conv_intrinsic_cobound (gfc_se * se, gfc_expr * expr) if (INTEGER_CST_P (bound)) { - int hi, low; - - hi = TREE_INT_CST_HIGH (bound); - low = TREE_INT_CST_LOW (bound); - if (hi || low < 1 || low > GFC_TYPE_ARRAY_CORANK (TREE_TYPE (desc))) + if (wi::ltu_p (bound, 1) + || wi::gtu_p (bound, GFC_TYPE_ARRAY_CORANK (TREE_TYPE (desc)))) gfc_error ("'dim' argument of %s intrinsic at %L is not a valid " "dimension index", expr->value.function.isym->name, &expr->where); diff --git a/gcc/fortran/trans-types.c b/gcc/fortran/trans-types.c index 21d9f28..f447155 100644 --- a/gcc/fortran/trans-types.c +++ b/gcc/fortran/trans-types.c @@ -863,8 +863,6 @@ gfc_init_types (void) int index; tree type; unsigned n; - unsigned HOST_WIDE_INT hi; - unsigned HOST_WIDE_INT lo; /* Create and name the types. */ #define PUSH_TYPE(name, node) \ @@ -956,13 +954,10 @@ gfc_init_types (void) descriptor. */ n = TYPE_PRECISION (gfc_array_index_type) - GFC_DTYPE_SIZE_SHIFT; - lo = ~ (unsigned HOST_WIDE_INT) 0; - if (n > HOST_BITS_PER_WIDE_INT) - hi = lo >> (2*HOST_BITS_PER_WIDE_INT - n); - else - hi = 0, lo >>= HOST_BITS_PER_WIDE_INT - n; gfc_max_array_element_size - = build_int_cst_wide (long_unsigned_type_node, lo, hi); + = wide_int_to_tree (long_unsigned_type_node, + wi::mask (n, UNSIGNED, + TYPE_PRECISION (long_unsigned_type_node))); boolean_type_node = gfc_get_logical_type (gfc_default_logical_kind); boolean_true_node = build_int_cst (boolean_type_node, 1); @@ -1895,7 +1890,7 @@ gfc_get_array_type_bounds (tree etype, int dimen, int codimen, tree * lbound, if (stride) rtype = build_range_type (gfc_array_index_type, gfc_index_zero_node, int_const_binop (MINUS_EXPR, stride, - integer_one_node)); + build_int_cst (TREE_TYPE (stride), 1))); else rtype = gfc_array_range_type; arraytype = build_array_type (etype, rtype);