| Message ID | 87r2x6rxp5.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 24, 2017 at 10:28:06AM +0100, Richard Sandiford wrote: > > So what does it change in the interfaces we use? I couldn't find an > > update of documentation, maybe I missed it (it's a huge series :-) ) > > An overview of the new interfaces (and how they are used) would help. > > You didn't miss one. I was hoping the function comments would be enough, > but on reflection, they're not. I've attached a patch for rtl.texi below. Thanks! > > From what I can tell so far it makes things much harder to read. > > Perhaps that is just because this is all new. > > Which parts specifically? E.g. is it mostly the is_a <T> (x, &y) changes? > Or the as_a <T> (x) changes too? Do you think the FOR_EACH_* iterators > also make things harder to read? Or is machine_mode->scalar_int_mode > itself a problem? All the as_a <T> (x) etc. looks like cuneiform to me (not just in your patch); and I cannot read cuneiform :-) One day I might understand why we need all this C++ inverted syntax, needless abstraction, needless generalisation, data hiding and everything else hiding. Until then, I rant. Sorry. The main purpose of abstraction is to make code easier to understand and to write and change, but with C++ it usually makes it harder it seems :-( Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Mon, Jul 24, 2017 at 10:28:06AM +0100, Richard Sandiford wrote: >> > From what I can tell so far it makes things much harder to read. >> > Perhaps that is just because this is all new. >> >> Which parts specifically? E.g. is it mostly the is_a <T> (x, &y) changes? >> Or the as_a <T> (x) changes too? Do you think the FOR_EACH_* iterators >> also make things harder to read? Or is machine_mode->scalar_int_mode >> itself a problem? > > All the as_a <T> (x) etc. looks like cuneiform to me (not just in your > patch); and I cannot read cuneiform :-) > > One day I might understand why we need all this C++ inverted syntax, > needless abstraction, needless generalisation, data hiding and everything > else hiding. Until then, I rant. Sorry. > > The main purpose of abstraction is to make code easier to understand and > to write and change, but with C++ it usually makes it harder it seems :-( Well, as someone who was/is on the fence about the C++ thing, I definitely sympathise :-) But in this particular case it really isn't (supposed to be) "hey, we're using C++ now, let's add more layers!" abstraction. The same principle would have worked in C and IMO been useful in C. It sounds like you don't necessarily object to SCALAR_INT_TYPE_MODE etc. as asserting forms of TYPE_MODE, so if the syntax is a problem, I think: as_a <scalar_int_mode> (x) => force_scalar_int (x) is_a <scalar_int> (x, &y) => is_scalar_int (x, &y) would be fine too, and shorter to write. Would that be better? Like I say, one advantage of the new wrappers is that they force mode assumptions to be explicit (otherwise the compiler won't build). That caught several real bugs before they had been found and fixed upstream. But that argument might be too weak to support this on its own. The other -- original, and IMO more compelling -- motivation is to make it easier to add variable-sized modes without having to worry about the possibility that scalar or complex modes could be variable-sized (because that would be much more invasive). We could instead have kept everything as machine_mode, made GET_MODE_SIZE always be variable, and forced the result to a constant whenever it's "known" to be constant for some reason. The difficulty with that is that it would be very hard to get right if not testing SVE, and even with SVE you would need just the right input code to trigger the problem. So what we wanted to do was make it "easy" for people to know when variable-sized modes were a concern even if they weren't thinking about or testing SVE. This scalar/not scalar or complex/not complex choices aren't architecture- specific in the same way. With this approach we only needed to force a variable size or offset to a constant in a few places, and those cases were easy to justify from context. Another alternative would have been to add functions like GET_SCALAR_MODE_SIZE that first assert that the mode is scalar and then return a constant size. However, that would have led to a lot more asserts in an enable-checking build and IMO wouldn't have been any more readable than an explicit "is this a scalar?" check followed by the normal GET_MODE_SIZE accessors. Thanks, Richard
On Mon, Jul 24, 2017 at 01:52:49PM +0100, Richard Sandiford wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Mon, Jul 24, 2017 at 10:28:06AM +0100, Richard Sandiford wrote: > >> > From what I can tell so far it makes things much harder to read. > >> > Perhaps that is just because this is all new. > >> > >> Which parts specifically? E.g. is it mostly the is_a <T> (x, &y) changes? > >> Or the as_a <T> (x) changes too? Do you think the FOR_EACH_* iterators > >> also make things harder to read? Or is machine_mode->scalar_int_mode > >> itself a problem? > > > > All the as_a <T> (x) etc. looks like cuneiform to me (not just in your > > patch); and I cannot read cuneiform :-) > > > > One day I might understand why we need all this C++ inverted syntax, > > needless abstraction, needless generalisation, data hiding and everything > > else hiding. Until then, I rant. Sorry. > > > > The main purpose of abstraction is to make code easier to understand and > > to write and change, but with C++ it usually makes it harder it seems :-( > > Well, as someone who was/is on the fence about the C++ thing, I definitely > sympathise :-) But in this particular case it really isn't (supposed to be) > "hey, we're using C++ now, let's add more layers!" abstraction. :-) > The same principle would have worked in C and IMO been useful in C. > It sounds like you don't necessarily object to SCALAR_INT_TYPE_MODE etc. > as asserting forms of TYPE_MODE, so if the syntax is a problem, I think: > > as_a <scalar_int_mode> (x) => force_scalar_int (x) > is_a <scalar_int> (x, &y) => is_scalar_int (x, &y) > > would be fine too, and shorter to write. Would that be better? Yeah that looks better, to me at least. > Like I say, one advantage of the new wrappers is that they force mode > assumptions to be explicit (otherwise the compiler won't build). That > caught several real bugs before they had been found and fixed upstream. > But that argument might be too weak to support this on its own. It also helps compile time you said. I like that, too :-) > The other -- original, and IMO more compelling -- motivation is to make > it easier to add variable-sized modes without having to worry about the > possibility that scalar or complex modes could be variable-sized > (because that would be much more invasive). We could instead have kept > everything as machine_mode, made GET_MODE_SIZE always be variable, and > forced the result to a constant whenever it's "known" to be constant > for some reason. The difficulty with that is that it would be very hard > to get right if not testing SVE, and even with SVE you would need just > the right input code to trigger the problem. So what we wanted to do > was make it "easy" for people to know when variable-sized modes were a > concern even if they weren't thinking about or testing SVE. This > scalar/not scalar or complex/not complex choices aren't architecture- > specific in the same way. > > With this approach we only needed to force a variable size or offset to > a constant in a few places, and those cases were easy to justify from > context. Yes, it is clear some changes are needed -- I just hope the changes can make the code easier to understand, instead of more complex. Segher
On July 24, 2017 3:42:30 PM GMT+02:00, Segher Boessenkool <segher@kernel.crashing.org> wrote: >On Mon, Jul 24, 2017 at 01:52:49PM +0100, Richard Sandiford wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> > On Mon, Jul 24, 2017 at 10:28:06AM +0100, Richard Sandiford wrote: >> >> > From what I can tell so far it makes things much harder to read. >> >> > Perhaps that is just because this is all new. >> >> >> >> Which parts specifically? E.g. is it mostly the is_a <T> (x, &y) >changes? >> >> Or the as_a <T> (x) changes too? Do you think the FOR_EACH_* >iterators >> >> also make things harder to read? Or is >machine_mode->scalar_int_mode >> >> itself a problem? >> > >> > All the as_a <T> (x) etc. looks like cuneiform to me (not just in >your >> > patch); and I cannot read cuneiform :-) >> > >> > One day I might understand why we need all this C++ inverted >syntax, >> > needless abstraction, needless generalisation, data hiding and >everything >> > else hiding. Until then, I rant. Sorry. >> > >> > The main purpose of abstraction is to make code easier to >understand and >> > to write and change, but with C++ it usually makes it harder it >seems :-( >> >> Well, as someone who was/is on the fence about the C++ thing, I >definitely >> sympathise :-) But in this particular case it really isn't (supposed >to be) >> "hey, we're using C++ now, let's add more layers!" abstraction. > >:-) > >> The same principle would have worked in C and IMO been useful in C. >> It sounds like you don't necessarily object to SCALAR_INT_TYPE_MODE >etc. >> as asserting forms of TYPE_MODE, so if the syntax is a problem, I >think: >> >> as_a <scalar_int_mode> (x) => force_scalar_int (x) >> is_a <scalar_int> (x, &y) => is_scalar_int (x, &y) >> >> would be fine too, and shorter to write. Would that be better? > >Yeah that looks better, to me at least. I don't like that. We've settled on one style so please adhere to that. Force_ also suggests some magic semantics that are not there. So if any then try to improve the as-is stuff in general. For example it's quite awkward in switches. Richard. >> Like I say, one advantage of the new wrappers is that they force mode >> assumptions to be explicit (otherwise the compiler won't build). >That >> caught several real bugs before they had been found and fixed >upstream. >> But that argument might be too weak to support this on its own. > >It also helps compile time you said. I like that, too :-) > >> The other -- original, and IMO more compelling -- motivation is to >make >> it easier to add variable-sized modes without having to worry about >the >> possibility that scalar or complex modes could be variable-sized >> (because that would be much more invasive). We could instead have >kept >> everything as machine_mode, made GET_MODE_SIZE always be variable, >and >> forced the result to a constant whenever it's "known" to be constant >> for some reason. The difficulty with that is that it would be very >hard >> to get right if not testing SVE, and even with SVE you would need >just >> the right input code to trigger the problem. So what we wanted to do >> was make it "easy" for people to know when variable-sized modes were >a >> concern even if they weren't thinking about or testing SVE. This >> scalar/not scalar or complex/not complex choices aren't architecture- >> specific in the same way. >> >> With this approach we only needed to force a variable size or offset >to >> a constant in a few places, and those cases were easy to justify from >> context. > >Yes, it is clear some changes are needed -- I just hope the changes >can make the code easier to understand, instead of more complex. > > >Segher
Index: gcc/doc/rtl.texi =================================================================== --- gcc/doc/rtl.texi 2017-07-24 10:11:36.155904178 +0100 +++ gcc/doc/rtl.texi 2017-07-24 10:11:40.484732386 +0100 @@ -1402,6 +1402,108 @@ classes. Currently @code{VOIDmode} and @code{MODE_RANDOM}. @end table +@cindex machine mode wrapper classes +@code{machmode.h} also defines various wrapper classes that combine a +@code{machine_mode} with a static assertion that a particular +condition holds. The classes are: + +@table @code +@findex scalar_int_mode +@item scalar_int_mode +A mode that has class @code{MODE_INT} or @code{MODE_PARTIAL_INT}. + +@findex scalar_float_mode +@item scalar_float_mode +A mode that has class @code{MODE_FLOAT} or @code{MODE_DECIMAL_FLOAT}. + +@findex scalar_mode +@item scalar_mode +A mode that holds a single numerical value. In practice this means +that the mode is a @code{scalar_int_mode}, is a @code{scalar_float_mode}, +or has class @code{MODE_FRACT}, @code{MODE_UFRACT}, @code{MODE_ACCUM}, +@code{MODE_UACCUM} or @code{MODE_POINTER_BOUNDS}. + +@findex complex_mode +@item complex_mode +A mode that has class @code{MODE_COMPLEX_INT} or @code{MODE_COMPLEX_FLOAT}. +@end table + +Named modes use the most constrained of the available wrapper classes, +if one exists, otherwise they use @code{machine_mode}. For example, +@code{QImode} is a @code{scalar_int_mode}, @code{SFmode} is a +@code{scalar_float_mode} and @code{BLKmode} is a plain +@code{machine_mode}. It is possible to refer to any mode as a raw +@code{machine_mode} by adding the @code{E_} prefix, where @code{E} +stands for ``enumeration''. For example, the raw @code{machine_mode} +names of the modes just mentioned are @code{E_QImode}, @code{E_SFmode} +and @code{E_BLKmode} respectively. + +The wrapper classes implicitly convert to @code{machine_mode} and to any +wrapper class that represents a more general condition; for example +@code{scalar_int_mode} and @code{scalar_float_mode} both convert +to @code{scalar_mode}. The classes act like @code{machine_mode}s +that accept only certain named modes. + +@findex opt_mode +@file{machmode.h} also defines a template class @code{opt_mode<@var{T}>} +that holds a @code{T} or nothing, where @code{T} can be either +@code{machine_mode} or one of the wrapper classes above. The main +operations on an @code{opt_mode<@var{T}>} @var{x} are as follows: + +@table @samp +@item @var{x}.exists () +Return true if @var{x} holds a mode rather than nothing. + +@item @var{x}.exists (&@var{y}) +Return true if @var{x} holds a mode rather than nothing, storing the +mode in @var{y} if so. @var{y} must be assignment-compatible with @var{T}. + +@item *@var{x} +Assert that @var{x} holds a mode rather than nothing and return that mode. + +@item @var{x} = @var{y} +Set @var{x} to @var{y}, where @var{y} is a @var{T} or implicitly converts +to a @var{T}. +@end table + +The default constructor sets an @code{opt_mode<@var{T}>} to nothing. +There is also a constructor that takes an initial value of type @var{T}. + +It is possible to use the @file{is-a.h} accessors on a @code{machine_mode} +or machine mode wrapper @var{x}: + +@table @samp +@findex is_a +@item is_a <@var{T}> (@var{x}) +Return true if @var{x} meets the conditions for wrapper class @var{T}. + +@item is_a <@var{T}> (@var{x}, &@var{y}) +Return true if @var{x} meets the conditions for wrapper class @var{T}, +storing it in @var{y} if so. @var{y} must be assignment-compatible with +@var{T}. + +@item as_a <@var{T}> (@var{x}) +Assert that @var{x} meets the conditions for wrapper class @var{T} +and return it as a @var{T}. + +@item dyn_cast <@var{T}> (@var{x}) +Return an @code{opt_mode<@var{T}>} that holds @var{x} if @var{x} meets +the conditions for wrapper class @var{T} and that holds nothing otherwise. +@end table + +The purpose of these wrapper classes is to give stronger static type +checking. For example, if a function takes a @code{scalar_int_mode}, +a caller that has a general @code{machine_mode} must either check or +assert that the code is indeed a scalar integer first, using one of +the functions above. + +The wrapper classes are normal C++ classes, with user-defined +constructors. Sometimes it is useful to have a POD version of +the same type, particularly if the type appears in a @code{union}. +The template class @code{pod_mode<@var{T}>} provides a POD version +of wrapper class @var{T}. It is assignment-compatible with @var{T} +and implicitly converts to both @code{machine_mode} and @var{T}. + Here are some C macros that relate to machine modes: @table @code