Re: [0/67] Add wrapper classes for machine_modes

[Richard Sandiford <richard.sandiford@arm.com>]

Richard Biener <richard.guenther@gmail.com> writes:
> On Fri, Dec 9, 2016 at 1:48 PM, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>> This series includes most of the changes in group C from:
>>
>>     https://gcc.gnu.org/ml/gcc/2016-11/msg00033.html
>>
>> The idea is to add wrapper classes around machine_mode_enum
>> for specific groups of modes, such as scalar integers, scalar floats,
>> complex values, etc.  This has two main benefits: one specific to SVE
>> and one not.
>>
>> The SVE-specific benefit is that it helps to introduce the concept
>> of variable-length vectors.  To do that we need to change the size
>> of a vector mode from being a known compile-time constant to being
>> (possibly) a run-time invariant.  We then need to do the same for
>> unconstrained machine_modes, which might or might not be vectors.
>> Introducing these new constrained types means that we can continue
>> to treat them as having a constant size.
>>
>> The other benefit is that it uses static type checking to enforce
>> conditions that are easily forgotten otherwise.  The most common
>> sources of problems seem to be:
>>
>> (a) using VOIDmode or BLKmode where a scalar integer was expected
>>     (e.g. when getting the number of bits in the value).
>>
>> (b) simplifying vector operations in ways that only make sense for
>>     scalars.
>>
>> The series helps with both of these, although we don't get the full
>> benefit of (b) until variable-sized modes are introduced.
>>
>> I know of three specific cases in which the static type checking
>> forced fixes for things that turned out to be real bugs (although
>> we didn't know that at the time, otherwise we'd have posted patches).
>> They were later fixed for trunk by:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01783.html
>>   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02983.html
>>   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02896.html
>>
>> The group C patches in ARM/sve-branch did slow compile time down a little.
>> I've since taken steps to avoid that:
>>
>> - Make the tailcall pass handle aggregate parameters and return values
>>   (already in trunk).
>>
>> - Turn some of the new wrapper functions into inline functions.
>>
>> - Make all the machmode.h macros that used:
>>
>>     __builtin_constant_p (M) ? foo_inline (M) : foo_array[M[
>>
>>   forward to an ALWAYS_INLINE function, so that (a) M is only evaluated
>>   once and (b) __builtin_constant_p is applied to a variable, and so is
>>   deferred until later passes.  This helped the optimisation to fire in
>>   more cases and to continue firing when M is a class rather than a
>>   raw enum.
>>
>> - In a similar vein, make sure that conditions like:
>>
>>      SImode == DImode
>>
>>   are treated as builtin_constant_p by gencondmd, so that .md patterns
>>   with those conditions are dropped.
>>
>> With these changes the series is actually a very slight compile-time win.
>> That might seem unlikely, but there are several possible reasons:
>>
>> 1. The machmode.h macro change above might allow more constant folding.
>>
>> 2. The series has a tendency to evaluate modes once, rather than
>>    continually fetching them from (sometimes quite deep) rtx nests.
>>    Refetching a mode is a particular problem if call comes between
>>    two uses, since the compiler then has to re-evaluate the whole thing.
>>
>> 3. The series introduces many uses of new SCALAR_*TYPE_MODE macros,
>>    as alternatives to TYPE_MODE.  The new macros avoid the usual:
>>
>>      (VECTOR_TYPE_P (TYPE_CHECK (NODE)) \
>>       ? vector_type_mode (NODE) : (NODE)->type_common.mode)
>>
>>    and become direct field accesses in release builds.
>>
>>    VECTOR_TYPE_P would be consistently false for these uses,
>>    but call-clobbered registers would usually be treated as clobbered
>>    by the condition as a whole.
>>
>> Maybe (3) is the most likely reason.
>>
>> I tested this by compiling the testsuite for:
>>
>>     aarch64-linux-gnu alpha-linux-gnu arc-elf arm-linux-gnueabi
>>     arm-linux-gnueabihf avr-elf bfin-elf c6x-elf cr16-elf cris-elf
>>     epiphany-elf fr30-elf frv-linux-gnu ft32-elf h8300-elf
>>     hppa64-hp-hpux11.23 ia64-linux-gnu i686-pc-linux-gnu
>>     i686-apple-darwin iq2000-elf lm32-elf m32c-elf m32r-elf
>>     m68k-linux-gnu mcore-elf microblaze-elf mips-linux-gnu
>>     mipsisa64-linux-gnu mmix mn10300-elf moxie-rtems msp430-elf
>>     nds32le-elf nios2-linux-gnu nvptx-none pdp11 powerpc-linux-gnuspe
>>     powerpc-eabispe powerpc64-linux-gnu powerpc-ibm-aix7.0 rl78-elf
>>     rx-elf s390-linux-gnu s390x-linux-gnu sh-linux-gnu sparc-linux-gnu
>>     sparc64-linux-gnu sparc-wrs-vxworks spu-elf tilegx-elf tilepro-elf
>>     xstormy16-elf v850-elf vax-netbsdelf visium-elf x86_64-darwin
>>     x86_64-linux-gnu xtensa-elf
>>
>> and checking that there were no changes in assembly.  Also tested
>> in the normal way on aarch64-linux-gnu and x86_64-linux-gnu.
>>
>> The series depends on the already-posted:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01657.html
>>
>> Sorry that this is so late, been distracted by other things.  Even if
>> we're too far into stage 3 for SVE itself to go in, I was hoping this
>> part (which was kind-of posted during stage 1) could go in independently.
>
> What's the benefit of taking this but not SVE?

That's what the third paragraph onwards was addressing.  It uses
static type checking to avoid a relatively common source of bugs,
with the "proof" being that we'd accidentally fixed at least three
PRs without knowing about it.  Note that the final two of the patches
I linked to above were only from the end of last month.

There's also the beneefit that it's overall a slight compile-time
improvement.

Thanks,
Richard