Re: Add a loop versioning pass

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

David Malcolm <dmalcolm@redhat.com> writes:
> On Wed, 2018-10-24 at 14:05 +0100, Richard Sandiford wrote:
>> This patch adds a pass that versions loops with variable index
>> strides
>> for the case in which the stride is 1.  E.g.:
>> 
>>     for (int i = 0; i < n; ++i)
>>       x[i * stride] = ...;
>> 
>> becomes:
>> 
>>     if (stepx == 1)
>>       for (int i = 0; i < n; ++i)
>>         x[i] = ...;
>>     else
>>       for (int i = 0; i < n; ++i)
>>         x[i * stride] = ...;
>> 
>> This is useful for both vector code and scalar code, and in some
>> cases
>> can enable further optimisations like loop interchange or pattern
>> recognition.
>> 
>> The pass gives a 7.6% improvement on Cortex-A72 for 554.roms_r at -O3
>> and a 2.4% improvement for 465.tonto.  I haven't found any SPEC tests
>> that regress.
>> 
>> Sizewise, there's a 10% increase in .text for both 554.roms_r and
>> 465.tonto.  That's obviously a lot, but in tonto's case it's because
>> the whole program is written using assumed-shape arrays and pointers,
>> so a large number of functions really do benefit from versioning.
>> roms likewise makes heavy use of assumed-shape arrays, and that
>> improvement in performance IMO justifies the code growth.
>> 
>> The next biggest .text increase is 4.5% for 548.exchange2_r.  I did
>> see
>> a small (0.4%) speed improvement there, but although both 3-iteration 
>> runs
>> produced stable results, that might still be noise.  There was a
>> slightly
>> larger (non-noise) improvement for a 256-bit SVE model.
>> 
>> 481.wrf and 521.wrf_r .text grew by 2.8% and 2.5% respectively, but
>> without any noticeable improvement in performance.  No other test
>> grew
>> by more than 2%.
>> 
>> Although the main SPEC beneficiaries are all Fortran tests, the
>> benchmarks we use for SVE also include some C and C++ tests that
>> benefit.
>> 
>> Using -frepack-arrays gives the same benefits in many Fortran cases.
>> The problem is that using that option inappropriately can force a
>> full
>> array copy for arguments that the function only reads once, and so it
>> isn't really something we can turn on by default.  The new pass is
>> supposed to give most of the benefits of -frepack-arrays without
>> the risk of unnecessary repacking.
>> 
>> The patch therefore enables the pass by default at -O3.
>> 
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
>> 
>> Richard
>> 
>
> [...snip...]
>
>> +/* Run the pass and return a set of TODO_* flags.  */
>> +
>> +unsigned int
>> +loop_versioning::run ()
>> +{
>> +  gcc_assert (scev_initialized_p ());
>> +
>> +  if (!analyze_blocks ()
>> +      || !prune_conditions ()
>> +      || !make_versioning_decisions ()
>> +      || !implement_versioning_decisions ())
>> +    return 0;
>> +
>> +  return TODO_update_ssa;
>> +}
>> +
>> +/* Loop versioningting pass.  */
>
> (typo)

Huh, no idea how I even got there.

>> +
>> +namespace {
>
> Could the whole file be within this anonymous namespace, rather than
> just the opt_pass subclass?  (hiding class loop_versioning, so that the
> optimizer knows that the only thing visible outside the TU is
> make_pass_loop_versioning).  This can be a pain to debug, but you can
> always comment out the anon namespace locally when debugging.

Yeah, I prefer that style too, so that the TU only exports public
interfaces.  I'd got the impression from earlier threads that this
was frowned on for GCC and so I was deliberately avoiding it, but if
it's OK then great.

Thanks,
Richard