diff mbox

Fix ICE on loop over constant vector at -O

Message ID 1486156.RQZsauqaQ0@polaris
State New
Headers show

Commit Message

Eric Botcazou Dec. 8, 2012, 11:42 a.m. UTC
This is an internal error in for_each_index at -O:

+===========================GNAT BUG DETECTED==============================+
| 4.8.0 20121208 (experimental) [trunk revision 194319] (x86_64-suse-linux) 
GCC error:|
| in for_each_index, at tree-ssa-loop-im.c:324                             |
| Error detected around vect9.adb:45:1|

The callback is invoked on a vector CONST_DECL and stops.  That's an unusual 
situation (the CONST_DECL is wrapped up in a VIEW_CONVERT_EXPR to array type) 
but IMO there is no reason to accept other DECLs and stop on CONST_DECLs.

Tested on x86_64-suse-linux, applied on the mainline as obvious.


2012-12-08  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-ssa-loop-im.c (for_each_index) <CONST_DECL>: New case.


2012-12-08  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/vect9.ad[sb]: New test.
	* gnat.dg/vect9_pkg.ads: New helper.

Comments

Richard Biener Dec. 10, 2012, 9:18 a.m. UTC | #1
On Sat, Dec 8, 2012 at 12:42 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> This is an internal error in for_each_index at -O:
>
> +===========================GNAT BUG DETECTED==============================+
> | 4.8.0 20121208 (experimental) [trunk revision 194319] (x86_64-suse-linux)
> GCC error:|
> | in for_each_index, at tree-ssa-loop-im.c:324                             |
> | Error detected around vect9.adb:45:1|
>
> The callback is invoked on a vector CONST_DECL and stops.  That's an unusual
> situation (the CONST_DECL is wrapped up in a VIEW_CONVERT_EXPR to array type)
> but IMO there is no reason to accept other DECLs and stop on CONST_DECLs.
>
> Tested on x86_64-suse-linux, applied on the mainline as obvious.

Well ... I would have expected that we'd have folded the CONST_DECL to
its DECL_INITIAL.  At least that is what we do for all other CONST_DECLs.
The only way CONST_DECLs should appear in the IL (after some optimization
of course) is when you take their address.

So - if you can burn some extra cycles to figure out why it's not
replaced by its DECL_INITIAL ...?

Richard.

> 2012-12-08  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-ssa-loop-im.c (for_each_index) <CONST_DECL>: New case.
>
>
> 2012-12-08  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gnat.dg/vect9.ad[sb]: New test.
>         * gnat.dg/vect9_pkg.ads: New helper.
>
>
> --
> Eric Botcazou
Eric Botcazou Dec. 10, 2012, 9:55 a.m. UTC | #2
> Well ... I would have expected that we'd have folded the CONST_DECL to
> its DECL_INITIAL.  At least that is what we do for all other CONST_DECLs.
> The only way CONST_DECLs should appear in the IL (after some optimization
> of course) is when you take their address.

That's what (essentially) happens here.  The CONST_DECL is wrapped up in a 
VIEW_CONVERT_EXPR to array type and a component with non-constant index is 
extracted from it.

> So - if you can burn some extra cycles to figure out why it's not
> replaced by its DECL_INITIAL ...?

We specifically prevent that in gigi because it will need to be spilled to 
memory given the above access pattern, so I don't really see the problem.
Here's the access in .optimized:

_38 = MEM[symbol: pkg__zero_unit, index: ivtmp.38_27, step: 4, offset: -4B];
Richard Biener Dec. 10, 2012, 12:28 p.m. UTC | #3
On Mon, Dec 10, 2012 at 10:55 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Well ... I would have expected that we'd have folded the CONST_DECL to
>> its DECL_INITIAL.  At least that is what we do for all other CONST_DECLs.
>> The only way CONST_DECLs should appear in the IL (after some optimization
>> of course) is when you take their address.
>
> That's what (essentially) happens here.  The CONST_DECL is wrapped up in a
> VIEW_CONVERT_EXPR to array type and a component with non-constant index is
> extracted from it.

Ah, ok ...

>> So - if you can burn some extra cycles to figure out why it's not
>> replaced by its DECL_INITIAL ...?
>
> We specifically prevent that in gigi because it will need to be spilled to
> memory given the above access pattern, so I don't really see the problem.
> Here's the access in .optimized:
>
> _38 = MEM[symbol: pkg__zero_unit, index: ivtmp.38_27, step: 4, offset: -4B];

... so if would really be a pessimization doing that.  Of course handling
CONST_DECL in for_each_index is indeed obvious - I just was curios if
it was a missed-optimization opportunity as well.

Thanks for clarifying,
Richard.

> --
> Eric Botcazou
diff mbox

Patch

Index: tree-ssa-loop-im.c
===================================================================
--- tree-ssa-loop-im.c	(revision 194319)
+++ tree-ssa-loop-im.c	(working copy)
@@ -291,6 +291,7 @@  for_each_index (tree *addr_p, bool (*cbc
 
 	case VAR_DECL:
 	case PARM_DECL:
+	case CONST_DECL:
 	case STRING_CST:
 	case RESULT_DECL:
 	case VECTOR_CST: