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

[Richard Sandiford <richard dot sandiford at linaro dot org>]

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