Patchwork Combiner fixes

login
register
mail settings
Submitter Bernd Schmidt
Date Aug. 4, 2010, 4:30 p.m.
Message ID <4C5995A6.8050701@codesourcery.com>
Download mbox | patch
Permalink /patch/60871/
State New
Headers show

Comments

Bernd Schmidt - Aug. 4, 2010, 4:30 p.m.
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.
H.J. Lu - Aug. 4, 2010, 5:45 p.m.
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.
Bernd Schmidt - Aug. 4, 2010, 5:49 p.m.
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
H.J. Lu - Aug. 4, 2010, 6 p.m.
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.
Bernd Schmidt - Aug. 4, 2010, 6:02 p.m.
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
H.J. Lu - Aug. 4, 2010, 6:08 p.m.
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.
Bernd Schmidt - Aug. 4, 2010, 9:10 p.m.
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
Richard Guenther - Aug. 7, 2010, 1:14 p.m.
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
>

Patch

Index: combine.c
===================================================================
--- combine.c	(revision 162849)
+++ combine.c	(working copy)
@@ -7093,7 +7093,9 @@  make_compound_operation (rtx x, enum rtx
      address, we stay there.  If we have a comparison, set to COMPARE,
      but once inside, go back to our default of SET.  */
 
-  next_code = (code == MEM || code == PLUS || code == MINUS ? MEM
+  next_code = (code == MEM ? MEM
+	       : ((code == PLUS || code == MINUS)
+		  && SCALAR_INT_MODE_P (mode)) ? MEM
 	       : ((code == COMPARE || COMPARISON_P (x))
 		  && XEXP (x, 1) == const0_rtx) ? COMPARE
 	       : in_code == COMPARE ? SET : in_code);
@@ -7127,8 +7129,8 @@  make_compound_operation (rtx x, enum rtx
     case PLUS:
       lhs = XEXP (x, 0);
       rhs = XEXP (x, 1);
-      lhs = make_compound_operation (lhs, MEM);
-      rhs = make_compound_operation (rhs, MEM);
+      lhs = make_compound_operation (lhs, next_code);
+      rhs = make_compound_operation (rhs, next_code);
       if (GET_CODE (lhs) == MULT && GET_CODE (XEXP (lhs, 0)) == NEG
 	  && SCALAR_INT_MODE_P (mode))
 	{
@@ -7157,8 +7159,8 @@  make_compound_operation (rtx x, enum rtx
     case MINUS:
       lhs = XEXP (x, 0);
       rhs = XEXP (x, 1);
-      lhs = make_compound_operation (lhs, MEM);
-      rhs = make_compound_operation (rhs, MEM);
+      lhs = make_compound_operation (lhs, next_code);
+      rhs = make_compound_operation (rhs, next_code);
       if (GET_CODE (rhs) == MULT && GET_CODE (XEXP (rhs, 0)) == NEG
 	  && SCALAR_INT_MODE_P (mode))
 	{
Index: testsuite/gcc.c-torture/compile/pr45182.c
===================================================================
--- testsuite/gcc.c-torture/compile/pr45182.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/pr45182.c	(revision 0)
@@ -0,0 +1,10 @@ 
+typedef struct TypHeader {
+  struct TypHeader ** ptr;
+} *TypHandle;
+void PlainRange (TypHandle hdList, long lenList, long low, long inc)
+{
+  long i;
+  for (i = 1; i <= lenList; i++ )
+    (((TypHandle*)((hdList)->ptr))[i] = (((TypHandle) (((long)(low + (i-1) *
+inc) << 2) + 1))));
+}