Re: [00/77] Add wrapper classes for machine_modes

[Richard Biener <richard dot guenther at gmail dot com>]

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