Combiner fixes

On 08/04/2010 05:39 PM, H.J. Lu wrote:
> This caused:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45182

This seems to be a latent bug.  We call make_compound_operation on

(plus:V2DI (ashift:V2DI (reg:V2DI 137)
        (const_int 2 [0x2]))
    (reg:V2DI 145))

and try to convert the shift into a multiplication, and
trunc_int_for_mode fails when asked to do something with V2DImode.

I think it's best not to do any of that for anything but plain integer
modes.  Ok if tests pass?


Bernd
PR middle-end/45182
	* combine.c (make_compound_operation): Don't try to convert
	shifts into multiplications for modes that aren't SCALAR_INT_MODE_P.

	PR middle-end/45182
	* gcc.c-torture/compile/pr45182.c: New test.

On Wed, Aug 4, 2010 at 9:30 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 08/04/2010 05:39 PM, H.J. Lu wrote:
>> This caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45182
>
> This seems to be a latent bug.  We call make_compound_operation on
>
> (plus:V2DI (ashift:V2DI (reg:V2DI 137)
>        (const_int 2 [0x2]))
>    (reg:V2DI 145))
>
> and try to convert the shift into a multiplication, and
> trunc_int_for_mode fails when asked to do something with V2DImode.
>
> I think it's best not to do any of that for anything but plain integer
> modes.  Ok if tests pass?
>

Doesn't this testcase require vectorizer? We need to ensure that
vectorizer is effective for both 32bit and 64bit on x86.

On 08/04/2010 07:45 PM, H.J. Lu wrote:

> Doesn't this testcase require vectorizer? We need to ensure that
> vectorizer is effective for both 32bit and 64bit on x86.

It failed at just -O3.  Not sure what you mean.


Bernd

On Wed, Aug 4, 2010 at 10:49 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 08/04/2010 07:45 PM, H.J. Lu wrote:
>
>> Doesn't this testcase require vectorizer? We need to ensure that
>> vectorizer is effective for both 32bit and 64bit on x86.
>
> It failed at just -O3.  Not sure what you mean.
>
>

From your description, it is due to

(plus:V2DI (ashift:V2DI (reg:V2DI 137)
       (const_int 2 [0x2]))
   (reg:V2DI 145))

The testcase is a scalar code. Only vectorizer will generate
V2DI. On ia32, gcc -O3 won't generate V2DI by default:

[hjl@gnu-35 delta]$
/net/gnu-1/export/gnu/import/svn/gcc-test/bld/gcc/xgcc
-B/net/gnu-1/export/gnu/import/svn/gcc-test/bld/gcc/ -S -O3
-ffast-math -funroll-loops  pr45182.c -march=i686
[hjl@gnu-35 delta]$
/net/gnu-1/export/gnu/import/svn/gcc-test/bld/gcc/xgcc
-B/net/gnu-1/export/gnu/import/svn/gcc-test/bld/gcc/ -S -O3
-ffast-math -funroll-loops  pr45182.c -march=i686 -msse2
pr45182.c: In function ‘PlainRange’:
pr45182.c:9:1: internal compiler error: in trunc_int_for_mode, at explow.c:57
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
[hjl@gnu-35 delta]$

On ia32, you need to enable SSE2 to see the failure.

On 08/04/2010 08:00 PM, H.J. Lu wrote:
> On ia32, you need to enable SSE2 to see the failure.

Well, you can always add that to your TORTURE_OPTIONS for testing.


Bernd

On Wed, Aug 4, 2010 at 11:02 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 08/04/2010 08:00 PM, H.J. Lu wrote:
>> On ia32, you need to enable SSE2 to see the failure.
>
> Well, you can always add that to your TORTURE_OPTIONS for testing.
>

We can't ask everyone to add "-msse2" to TORTURE_OPTIONS on ia32.
Testcase in PR45182 only fails with -O3 -msse2 on ia32. If we don't add -msse2,
we may not know it is broken again in the future.

On 08/04/2010 08:08 PM, H.J. Lu wrote:
> On Wed, Aug 4, 2010 at 11:02 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>> On 08/04/2010 08:00 PM, H.J. Lu wrote:
>>> On ia32, you need to enable SSE2 to see the failure.
>>
>> Well, you can always add that to your TORTURE_OPTIONS for testing.
>>
> 
> We can't ask everyone to add "-msse2" to TORTURE_OPTIONS on ia32.
> Testcase in PR45182 only fails with -O3 -msse2 on ia32. If we don't add -msse2,
> we may not know it is broken again in the future.

I'd expect some people also test x86_64 where it's covered by -O3.
Placing the testcase in gcc.c-torture gives us wider coverage IMO.  If
it's really necessary we can put a copy in gcc.target, but I don't
really see too much value in that.


Bernd

On Wed, Aug 4, 2010 at 11:10 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 08/04/2010 08:08 PM, H.J. Lu wrote:
>> On Wed, Aug 4, 2010 at 11:02 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>> On 08/04/2010 08:00 PM, H.J. Lu wrote:
>>>> On ia32, you need to enable SSE2 to see the failure.
>>>
>>> Well, you can always add that to your TORTURE_OPTIONS for testing.
>>>
>>
>> We can't ask everyone to add "-msse2" to TORTURE_OPTIONS on ia32.
>> Testcase in PR45182 only fails with -O3 -msse2 on ia32. If we don't add -msse2,
>> we may not know it is broken again in the future.
>
> I'd expect some people also test x86_64 where it's covered by -O3.
> Placing the testcase in gcc.c-torture gives us wider coverage IMO.  If
> it's really necessary we can put a copy in gcc.target, but I don't
> really see too much value in that.

Me neither.  The patch is ok.

Thanks,
Richard.

>
> Bernd
>