making the new if-converter not mangle IR that is already vectorizer-friendly

[Richard Biener]

On Tue, Jul 21, 2015 at 10:19 PM, Abe <abe_skolnik@yahoo.com> wrote:
> First: my apologies for the delay in this reply.

Likewise ;)

>
> [Richard wrote:]
>>
>> Well, but we do have a pretty strong if-converter  on RTL
>
>> which has access to target specific information.
>
> Yes, I have had a quick look at it.  It looks quite thorough.
>
> I think I see that you [Richard] are implying that the if converter
> at the GIMPLE level should not be trying to do all the if-conversion
> work that could possibly be done.  I agree with that.  However,
> AFAIK the RTL work is done strictly after the autovectorization,
> so any if conversion that is strictly for the benefit of
> autovectorization must be done before autovectorization
> and therefor at the GIMPLE level.  Corrections are welcome.

Yes, the GIMPLE if-converter should strictly be a vectorization enabler.
If the "new" if-converter produces code that the vectorizer does not
handle (on the target targeted) then it has done a bad job.

>
> [Abe wrote:]
>>>
>>> The preceding makes me wonder: has anybody considered adding an
>>> optimization
>>> profile for GCC, […] that optimizes for the amount of energy consumed?
>
>>> I don`t remember reading about anything like that […]
>
> [Richard wrote:]
>>
>> I think there were GCC summit papers/talks about this.
>
>
> Thanks, but can you please be more specific?
>
> After writing the message quotes above as "[Abe wrote:]"
> I found 2 or 3 papers about compiling code with an eye
> towards energy efficiency, but not a whole hell of a lot,
> and I didn`t yet find anything GCC-specific on this topic.
>
>
> [Abe wrote:]
>>>
>>> The old one can, in some cases, produce code that
>>> e.g. dereferences a null pointer when the same
>>> program given the same inputs would have not
>
>>> done so without the if-conversion "optimization".
>
> [Richard wrote:]
>>
>> Testcase?  I don't think it can and if it can this bug needs to be fixed.
>
>
> With the program below my sign-off, using stock GCC 4.9.2, even under "-O3"
> the code compiles and runs OK, even when using "-ftree-loop-if-convert",
> which is I guess what you [Richard] meant by a recent comment to me [Abe].
> Sebastian confirmed in person that even the old if converter did things
> differently
> even to loads when GCC is invoked with the full
> "-ftree-loop-if-convert-stores"
> flag but without "-ftree-loop-if-convert-stores".  With the old converter,
> compiling with "-ftree-loop-if-convert-stores" yields a program that
> segfaults due
> to dereferencing a null pointer [it would deref. a _lot_ of them if it
> _could_ ;-)].
>
> [tested using GCC 4.9.2 on both Cygwin (64-bit) for Windows 7, AMD64
> compilation,
>  and Mac OS X 10.6.8 -- also using GCC 4.9.2 -- compiling for both ia32 and
> AMD64]
>
> I intend to adapt the test case to DejaGNU format and add
> it to the codebase from which the patch is being generated.

I think I fixed this bug.  It was because of an inverted test.

2015-07-10  Richard Biener  <rguenther@suse.de>

        PR tree-optimization/66823
        * tree-if-conv.c (memrefs_read_or_written_unconditionally): Fix
        inverted predicate.

>
> [Abe wrote:]
>>>
>>> The new converter reduces/eliminates this problem.
>
>
> [Richard wrote:]
>>
>> You mean the -ftree-loop-if-convert-stores path.
>
>
> The old converter apparently only produced code with the aforementioned
> crashing problem only when "-ftree-loop-if-convert-stores" is/was in use,
> yes.

Due to a bug.  It was never supposed to do invalid transforms.  It was
known to produce store data-races with -ftree-loop-if-convert-stores.
That is something the scratchpad use avoids (at the expense of
a lot(?) of vectorization).

> The new one should not be producing code with that problem
> regardless of "-ftree-loop-if-convert-stores" or lack thereof.
>
> I think the reason for the confusing ambiguity is that since the old
> converter
> did conversion of stores in a way that was thread-unsafe for half hammocks
> [e.g. C source code like "if (condition)  A[a] = something;" with no
> attached
> "else", assuming "condition" and "something" are both conversion-friendly],
> somebody used "-ftree-loop-if-convert-stores" to mean "if-convert as much
> as possible even if doing so requires converting unsafely".  In the new
> converter we have no such unsafety TTBOMK, so I propose to remove that flag.

First of all to fix regressions on the load if-conversion side you should stop
using a scratchpad for loads (do you have one?  I seem to remember you
do from the design description).  load data-races are only theoretical.

Then you should IMHO keep the old store path for --param
allow-store-data-races=1.

I'd like to see a comparison in the number of vectorized loops for
SPEC CPU 2006 FP
with -Ofast -ftree-loop-if-convert-stores with / without the new if-converter
(and if you like also without -ftree-loop-if-convert-stores).  You can use
-fopt-info-vec and grep for the number of 'loop vectorized' cases.

Thanks,
Richard.

> Regards,
>
> Abe
>
>
>
>
>
> Makefile
> ========
> all: foo_______if-converted foo_______if-converted_with_stores_flag
> foo___NOT_if-converted
>
> foo_______if-converted_with_stores_flag: foo.c
>         gcc -std=c99 -O3 -ftree-loop-if-convert-stores foo.c -o
> foo_______if-converted_with_stores_flag
>
> foo_______if-converted: foo.c
>         gcc -std=c99 -O3 -ftree-loop-if-convert        foo.c -o
> foo_______if-converted
>
> foo___NOT_if-converted: foo.c
>         gcc -std=c99 -O3                               foo.c -o
> foo___NOT_if-converted
>
>
>
>
> foo.c
> =====
> /* intentionally not defining "SIZE" here:
>      pretending that "choose" is compiled separately from "main"
> */
>
> /* a "controlled copy-paste" that takes 4 arrays, 2 of which are full of
> pointers,
>    and for each index deref.s one of the 2 pointers and shoves the result in
> one
>    of the arrays, based on the value in the respective index of the
> remaining array
> */
>
> void __attribute__((noinline)) /* forcing no inlining b/c inlining
> theoretically allow
>                                   sufficient analysis to allow the optimizer
> to "see"
>                                   that "cond_array[...]" is full of nothing
> but 0s
>                                */
>   choose(char*       cond_array,
>          short ** pointer_array_1,
>          short ** pointer_array_2,
>          short *   output_array,
>          unsigned long long len) {
>
>   for (unsigned long long index = 0; index < len; ++index)
>     if (cond_array[index])
>       output_array[index] = *pointer_array_1[index];
>     else
>       output_array[index] = *pointer_array_2[index];
>
> }
>
>
>
> #define SIZE 9
>
> int main() {
>
>   char           condition[SIZE];
>   short           data_in [SIZE];
>   short           data_out[SIZE];
>   short *  pointer_array_1[SIZE];
>   short *  pointer_array_2[SIZE];
>
>   for (unsigned short index = 0; index < SIZE; ++index) {
>     condition      [index]  = 0;              /*** all false         ***/
>     pointer_array_1[index]  = 0;              /*** all null pointers ***/
>     pointer_array_2[index] = &data_in[index]; /*** all good pointers ***/
>   }
>
>   choose(&condition[0], &pointer_array_1[0], &pointer_array_2[0],
> &data_out[0], SIZE);
>
> }
>
>
>
>
>
>
>
> result from running MacOSX/ia32 compilation under GDB 6.3.50*
> =============================================================
> Program received signal EXC_BAD_ACCESS, Could not access memory.
> Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
> 0x00001e2b in choose ()
>
>
>
> result from running MacOSX/AMD64 compilation under GDB 6.3.50*
> ==============================================================
> Program received signal EXC_BAD_ACCESS, Could not access memory.
> Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
> 0x0000000100000e90 in choose ()
>
>
>
> * Apple-supplied version of GDB, identified as:
>
>     GNU gdb 6.3.50-20050815 (Apple version gdb-1515) (Sat Jan 15 08:33:48
> UTC 2011)