Patchwork [C++11,4.9] Add missing REDUC_PLUS_EXPR case to potential_constant_expression_1.

login
register
mail settings
Submitter James Greenhalgh
Date April 10, 2013, 10:50 a.m.
Message ID <1365591024-3517-1-git-send-email-james.greenhalgh@arm.com>
Download mbox | patch
Permalink /patch/235362/
State New
Headers show

Comments

James Greenhalgh - April 10, 2013, 10:50 a.m.
> -----Original Message-----
> From: dosreis@gmail.com [mailto:dosreis@gmail.com] On Behalf Of Gabriel
> Dos Reis
> Sent: 20 March 2013 19:09
> To: James Greenhalgh
> Cc: Jakub Jelinek; Richard Biener; gcc-patches@gcc.gnu.org; Jason
> Merrill; mark@codesourcery.com
> Subject: Re: [C++11][4.9] Add missing REDUC_PLUS_EXPR case to
> potential_constant_expression_1.
>
> On Wed, Mar 20, 2013 at 1:03 PM, James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
>
> > Is that be sensible? It certainly seems like someone intended to
> > explicitly enumerate all the possible cases and ensure that they were
> > correctly handled.
>
> That someone would be me.
>
> We need to catch loudly any front-end tree code, e.g. ASTs, object
> we may have missed, as opposed to silently ignoring them with
> possible miscompilation and pray that someone might be sufficiently
> pissed off and report it as a bug.
>
> What is wrong isn't that the front-end inserts internal coverage
> check; rather it is the fact that we don't have enough separation
> between front-end asts and middle-end stuff.
>
> The convenience of adding a middle-end optimization (which this
> essentially is) should not trump correctness of the implementation
> of standard semantics.

So, as far as I can see no decision came out of this thread as to
what should be done. In that time I had to add another few tree cases
as I added more things to TARGET_FOLD_BUILTIN.

I'd like to start pushing some of these TARGET_FOLD_BUILTIN patches
upstream, but they currently all hinge on resolving this discussion.

Would it be OK for this patch to go in, I know the thread started
well for me with:

> -----Original Message-----
> From: Jason Merrill [mailto:jason@redhat.com]
> Sent: 14 March 2013 18:52
> To: James Greenhalgh; gcc-patches@gcc.gnu.org
> Cc: mark@codesourcery.com
> Subject: Re: [C++11][4.9] Add missing REDUC_PLUS_EXPR case to
> potential_constant_expression_1.
>
> On 03/14/2013 09:48 AM, James Greenhalgh wrote:
> > Is this OK to commit to 4.9 when stage 1 opens up?
>
> Yes, but please add the other new tree codes as well.
>
> Jason

But quickly moved on to discussion, so I didn't commit the patch.

Thanks,
James Greenhalgh
Graduate Engineer
ARM

---
gcc/

2013-04-09  James Greenhalgh  <james.greenhalgh@arm.com>

	* cp/semantics.c
	(potential_constant_expression_1): Add cases for REDUC_PLUS_EXPR,
	REDUC_MIN_EXPR, REDUC_MAX_EXPR.
Richard Guenther - April 10, 2013, 11:31 a.m.
On Wed, Apr 10, 2013 at 12:50 PM, James Greenhalgh
<james.greenhalgh@arm.com> wrote:
>
>> -----Original Message-----
>> From: dosreis@gmail.com [mailto:dosreis@gmail.com] On Behalf Of Gabriel
>> Dos Reis
>> Sent: 20 March 2013 19:09
>> To: James Greenhalgh
>> Cc: Jakub Jelinek; Richard Biener; gcc-patches@gcc.gnu.org; Jason
>> Merrill; mark@codesourcery.com
>> Subject: Re: [C++11][4.9] Add missing REDUC_PLUS_EXPR case to
>> potential_constant_expression_1.
>>
>> On Wed, Mar 20, 2013 at 1:03 PM, James Greenhalgh
>> <james.greenhalgh@arm.com> wrote:
>>
>> > Is that be sensible? It certainly seems like someone intended to
>> > explicitly enumerate all the possible cases and ensure that they were
>> > correctly handled.
>>
>> That someone would be me.
>>
>> We need to catch loudly any front-end tree code, e.g. ASTs, object
>> we may have missed, as opposed to silently ignoring them with
>> possible miscompilation and pray that someone might be sufficiently
>> pissed off and report it as a bug.
>>
>> What is wrong isn't that the front-end inserts internal coverage
>> check; rather it is the fact that we don't have enough separation
>> between front-end asts and middle-end stuff.
>>
>> The convenience of adding a middle-end optimization (which this
>> essentially is) should not trump correctness of the implementation
>> of standard semantics.
>
> So, as far as I can see no decision came out of this thread as to
> what should be done. In that time I had to add another few tree cases
> as I added more things to TARGET_FOLD_BUILTIN.
>
> I'd like to start pushing some of these TARGET_FOLD_BUILTIN patches
> upstream, but they currently all hinge on resolving this discussion.

I still think getting rid of TARGET_FOLD_BUILTIN and replacing it with
TARGET_FOLD_STMT that only operates on GIMPLE is the way to go.

One of the issues we hit is that it's not well-defined what tree codes are
supposed to be part of GENERIC and which only part of GIMPLE
(in case we want to support GENERIC tree codes not being a superset
of GIMPLE tree codes at all).  If they are part of GENERIC then the
C++ frontend needs to handle them as folding can introduce all
GENERIC tree codes.

Richard.

> Would it be OK for this patch to go in, I know the thread started
> well for me with:
>
>> -----Original Message-----
>> From: Jason Merrill [mailto:jason@redhat.com]
>> Sent: 14 March 2013 18:52
>> To: James Greenhalgh; gcc-patches@gcc.gnu.org
>> Cc: mark@codesourcery.com
>> Subject: Re: [C++11][4.9] Add missing REDUC_PLUS_EXPR case to
>> potential_constant_expression_1.
>>
>> On 03/14/2013 09:48 AM, James Greenhalgh wrote:
>> > Is this OK to commit to 4.9 when stage 1 opens up?
>>
>> Yes, but please add the other new tree codes as well.
>>
>> Jason
>
> But quickly moved on to discussion, so I didn't commit the patch.
>
> Thanks,
> James Greenhalgh
> Graduate Engineer
> ARM
>
> ---
> gcc/
>
> 2013-04-09  James Greenhalgh  <james.greenhalgh@arm.com>
>
>         * cp/semantics.c
>         (potential_constant_expression_1): Add cases for REDUC_PLUS_EXPR,
>         REDUC_MIN_EXPR, REDUC_MAX_EXPR.
Tejas Belagod - April 15, 2013, 3:55 p.m.
Richard Biener wrote:
> On Wed, Apr 10, 2013 at 12:50 PM, James Greenhalgh
> <james.greenhalgh@arm.com> wrote:
>>> -----Original Message-----
>>> From: dosreis@gmail.com [mailto:dosreis@gmail.com] On Behalf Of Gabriel
>>> Dos Reis
>>> Sent: 20 March 2013 19:09
>>> To: James Greenhalgh
>>> Cc: Jakub Jelinek; Richard Biener; gcc-patches@gcc.gnu.org; Jason
>>> Merrill; mark@codesourcery.com
>>> Subject: Re: [C++11][4.9] Add missing REDUC_PLUS_EXPR case to
>>> potential_constant_expression_1.
>>>
>>> On Wed, Mar 20, 2013 at 1:03 PM, James Greenhalgh
>>> <james.greenhalgh@arm.com> wrote:
>>>
>>>> Is that be sensible? It certainly seems like someone intended to
>>>> explicitly enumerate all the possible cases and ensure that they were
>>>> correctly handled.
>>> That someone would be me.
>>>
>>> We need to catch loudly any front-end tree code, e.g. ASTs, object
>>> we may have missed, as opposed to silently ignoring them with
>>> possible miscompilation and pray that someone might be sufficiently
>>> pissed off and report it as a bug.
>>>
>>> What is wrong isn't that the front-end inserts internal coverage
>>> check; rather it is the fact that we don't have enough separation
>>> between front-end asts and middle-end stuff.
>>>
>>> The convenience of adding a middle-end optimization (which this
>>> essentially is) should not trump correctness of the implementation
>>> of standard semantics.
>> So, as far as I can see no decision came out of this thread as to
>> what should be done. In that time I had to add another few tree cases
>> as I added more things to TARGET_FOLD_BUILTIN.
>>
>> I'd like to start pushing some of these TARGET_FOLD_BUILTIN patches
>> upstream, but they currently all hinge on resolving this discussion.
> 
> I still think getting rid of TARGET_FOLD_BUILTIN and replacing it with
> TARGET_FOLD_STMT that only operates on GIMPLE is the way to go.
> 

Correct me if I'm wrong - as I currently understand the mid-end, 
TARGET_FOLD_BUILTIN could be used to fold builtins into tree expressions 
pre-vectorization so that the vectorizer could pick it up for optimization 
opportunities. Will folding to GIMPLE not defeat the purpose of folding builtins 
early on in GENERIC/Trees to expose them to early optimizations?

The main purpose of using TARGET_FOLD_BUILTIN was to fold NEON builtins to trees 
so that they could go through the vectorizer for optimizations. If we go 
directly to GIMPLE, won't we lose this advantage?

Thanks,
Tejas Belagod
ARM.

> One of the issues we hit is that it's not well-defined what tree codes are
> supposed to be part of GENERIC and which only part of GIMPLE
> (in case we want to support GENERIC tree codes not being a superset
> of GIMPLE tree codes at all).  If they are part of GENERIC then the
> C++ frontend needs to handle them as folding can introduce all
> GENERIC tree codes.
> 
> Richard.
> 
>> Would it be OK for this patch to go in, I know the thread started
>> well for me with:
>>
>>> -----Original Message-----
>>> From: Jason Merrill [mailto:jason@redhat.com]
>>> Sent: 14 March 2013 18:52
>>> To: James Greenhalgh; gcc-patches@gcc.gnu.org
>>> Cc: mark@codesourcery.com
>>> Subject: Re: [C++11][4.9] Add missing REDUC_PLUS_EXPR case to
>>> potential_constant_expression_1.
>>>
>>> On 03/14/2013 09:48 AM, James Greenhalgh wrote:
>>>> Is this OK to commit to 4.9 when stage 1 opens up?
>>> Yes, but please add the other new tree codes as well.
>>>
>>> Jason
>> But quickly moved on to discussion, so I didn't commit the patch.
>>
>> Thanks,
>> James Greenhalgh
>> Graduate Engineer
>> ARM
>>
>> ---
>> gcc/
>>
>> 2013-04-09  James Greenhalgh  <james.greenhalgh@arm.com>
>>
>>         * cp/semantics.c
>>         (potential_constant_expression_1): Add cases for REDUC_PLUS_EXPR,
>>         REDUC_MIN_EXPR, REDUC_MAX_EXPR.
>

Patch

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 72b884e..880f479 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -8619,6 +8619,9 @@  potential_constant_expression_1 (tree t, bool want_rval, tsubst_flags_t flags)
     case ABS_EXPR:
     case TRUTH_NOT_EXPR:
     case FIXED_CONVERT_EXPR:
+    case REDUC_PLUS_EXPR:
+    case REDUC_MIN_EXPR:
+    case REDUC_MAX_EXPR:
     case UNARY_PLUS_EXPR:
       return potential_constant_expression_1 (TREE_OPERAND (t, 0), rval,
 					      flags);