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

login
register
mail settings
Submitter James Greenhalgh
Date March 14, 2013, 1:48 p.m.
Message ID <1363268930-22910-1-git-send-email-james.greenhalgh@arm.com>
Download mbox | patch
Permalink /patch/227669/
State New
Headers show

Comments

James Greenhalgh - March 14, 2013, 1:48 p.m.
Hi,

As it stands, potential_constant_expression_1 does not handle
REDUC_PLUS_EXPR trees.

For some cross-lane add-reduce neon intrinsics we would like to
use the TARGET_FOLD_BUILTIN hook to fold these calls to a
REDUC_PLUS_EXPR. As an example, see Tejas Belagod's patch at:
http://gcc.gnu.org/ml/gcc-patches/2013-03/msg00502.html

If we do that, the C++11 front-end is not able to evaluate
them as a potential constant expression and we get an
"unexpected AST of kind REDUC_PLUS_EXPR" error.

I am about as far from a C++11 expert as you can get, so I
don't know whether this is the correct fix, but to my naieve
mind I don't see why REDUC_PLUS_EXPR is different from any
other unary operator...

I have tested the patch on aarch64-none-elf with no regressions,
and it fixed the problem I was seeing previously.

Is this OK to commit to 4.9 when stage 1 opens up?

Thanks,
James Greenhalgh

---
gcc/cp/

	* semantics.c
	(potential_constant_expression_1): Add case for REDUC_PLUS_EXPR.
Jason Merrill - March 14, 2013, 6:51 p.m.
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
Jakub Jelinek - March 14, 2013, 6:55 p.m.
On Thu, Mar 14, 2013 at 02:51:30PM -0400, Jason Merrill wrote:
> 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.

I wonder if it wouldn't be better to fold the target builtins only later on
(e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or if we have
any predicate that is set starting with gimplification or so)).
Having all the FEs have to deal with myriads of weird tree codes etc. isn't
IMHO desirable.

	Jakub
Gabriel Dos Reis - March 14, 2013, 8:47 p.m.
On Thu, Mar 14, 2013 at 1:55 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 14, 2013 at 02:51:30PM -0400, Jason Merrill wrote:
>> 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.
>
> I wonder if it wouldn't be better to fold the target builtins only later on
> (e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or if we have
> any predicate that is set starting with gimplification or so)).
> Having all the FEs have to deal with myriads of weird tree codes etc. isn't
> IMHO desirable.

Agreed.

-- Gaby
Marc Glisse - March 14, 2013, 9:08 p.m.
On Thu, 14 Mar 2013, Jakub Jelinek wrote:

> I wonder if it wouldn't be better to fold the target builtins only later on
> (e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or if we have
> any predicate that is set starting with gimplification or so)).
> Having all the FEs have to deal with myriads of weird tree codes etc. isn't
> IMHO desirable.

Wouldn't that prevent from using those builtins in constant expressions? 
That seems undesirable. Maybe an alternative could be to push some of the 
functionality from potential_constant_expression_1 to the middle-end?
Richard Guenther - March 15, 2013, 8:51 a.m.
On Thu, Mar 14, 2013 at 10:08 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 14 Mar 2013, Jakub Jelinek wrote:
>
>> I wonder if it wouldn't be better to fold the target builtins only later
>> on
>> (e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or if we have
>> any predicate that is set starting with gimplification or so)).
>> Having all the FEs have to deal with myriads of weird tree codes etc.
>> isn't
>> IMHO desirable.
>
>
> Wouldn't that prevent from using those builtins in constant expressions?
> That seems undesirable. Maybe an alternative could be to push some of the
> functionality from potential_constant_expression_1 to the middle-end?

True, but is that bad?

If we want to delay such folding then please don't do it with a magic flag
but instead do the folding only via fold_stmt - that is, add a new target hook
that folds a gimple call.  I bet we have only a very limited set of target call
foldings, so transitioning them all to fold gimple calls would be easy.

Richard.

> --
> Marc Glisse
Gabriel Dos Reis - March 15, 2013, 1 p.m.
On Fri, Mar 15, 2013 at 3:51 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Mar 14, 2013 at 10:08 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Thu, 14 Mar 2013, Jakub Jelinek wrote:
>>
>>> I wonder if it wouldn't be better to fold the target builtins only later
>>> on
>>> (e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or if we have
>>> any predicate that is set starting with gimplification or so)).
>>> Having all the FEs have to deal with myriads of weird tree codes etc.
>>> isn't
>>> IMHO desirable.
>>
>>
>> Wouldn't that prevent from using those builtins in constant expressions?
>> That seems undesirable. Maybe an alternative could be to push some of the
>> functionality from potential_constant_expression_1 to the middle-end?
>
> True, but is that bad?
>
> If we want to delay such folding then please don't do it with a magic flag
> but instead do the folding only via fold_stmt - that is, add a new target hook
> that folds a gimple call.  I bet we have only a very limited set of target call
> foldings, so transitioning them all to fold gimple calls would be easy.

upon reflection, I think we don't want to delay.  the C++ front-end
needs to know (while type checking) whether a certain operation
can be evaluated at compile time and possibly get the value of the
operation.

-- Gaby
Jakub Jelinek - March 15, 2013, 1:05 p.m.
On Fri, Mar 15, 2013 at 08:00:50AM -0500, Gabriel Dos Reis wrote:
> On Fri, Mar 15, 2013 at 3:51 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
> > On Thu, Mar 14, 2013 at 10:08 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> >> On Thu, 14 Mar 2013, Jakub Jelinek wrote:
> >>
> >>> I wonder if it wouldn't be better to fold the target builtins only later
> >>> on
> >>> (e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or if we have
> >>> any predicate that is set starting with gimplification or so)).
> >>> Having all the FEs have to deal with myriads of weird tree codes etc.
> >>> isn't
> >>> IMHO desirable.
> >>
> >>
> >> Wouldn't that prevent from using those builtins in constant expressions?
> >> That seems undesirable. Maybe an alternative could be to push some of the
> >> functionality from potential_constant_expression_1 to the middle-end?
> >
> > True, but is that bad?
> >
> > If we want to delay such folding then please don't do it with a magic flag
> > but instead do the folding only via fold_stmt - that is, add a new target hook
> > that folds a gimple call.  I bet we have only a very limited set of target call
> > foldings, so transitioning them all to fold gimple calls would be easy.
> 
> upon reflection, I think we don't want to delay.  the C++ front-end
> needs to know (while type checking) whether a certain operation
> can be evaluated at compile time and possibly get the value of the
> operation.

If all arguments to the target builtin constant, then it better not be
folded into REDUC_PLUS_EXPR, but instead just some constant.  That is just
fine.  If all arguments to the target builtin aren't constant, then it won't
be a constant expression anyway, there is no point in showing all those
weird tree codes to the FE.

	Jakub
James Greenhalgh - March 20, 2013, 6:03 p.m.
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: 15 March 2013 13:06
> On Fri, Mar 15, 2013 at 08:00:50AM -0500, Gabriel Dos Reis wrote:
> > On Fri, Mar 15, 2013 at 3:51 AM, Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > > On Thu, Mar 14, 2013 at 10:08 PM, Marc Glisse
> <marc.glisse@inria.fr> wrote:
> > >> On Thu, 14 Mar 2013, Jakub Jelinek wrote:
> > >>
> > >>> I wonder if it wouldn't be better to fold the target builtins
> only later
> > >>> on
> > >>> (e.g. guard the folding with cfun && gimple_in_ssa_p (cfun) (or
> if we have
> > >>> any predicate that is set starting with gimplification or so)).
> > >>> Having all the FEs have to deal with myriads of weird tree codes
> etc.
> > >>> isn't
> > >>> IMHO desirable.
> > >>
> > >>
> > >> Wouldn't that prevent from using those builtins in constant
> expressions?
> > >> That seems undesirable. Maybe an alternative could be to push some
> of the
> > >> functionality from potential_constant_expression_1 to the middle-
> end?
> > >
> > > True, but is that bad?
> > >
> > > If we want to delay such folding then please don't do it with a
> magic flag
> > > but instead do the folding only via fold_stmt - that is, add a new
> target hook
> > > that folds a gimple call.  I bet we have only a very limited set of
> target call
> > > foldings, so transitioning them all to fold gimple calls would be
> easy.
> >
> > upon reflection, I think we don't want to delay.  the C++ front-end
> > needs to know (while type checking) whether a certain operation
> > can be evaluated at compile time and possibly get the value of the
> > operation.
> 
> If all arguments to the target builtin constant, then it better not be
> folded into REDUC_PLUS_EXPR, but instead just some constant.  That is
> just
> fine.  If all arguments to the target builtin aren't constant, then it
> won't
> be a constant expression anyway, there is no point in showing all those
> weird tree codes to the FE.

So based on this argument, while a front-end may not need to implement
all the weird tree codes it should at least have a sensible default
case for an tree code which it doesn't understand.

That would suggest the correct fix would be to change
potential_constant_expression_1 to simply return false if it encounters
a tree it doesn't know about, rather than the current gcc_unreachable ().

The argument would be; if we found an unexpected tree code then
it must have been introduced by folding. If it was a constant,
folding would have returned the constant not the expression. So it must
not be a constant.

Is that be sensible? It certainly seems like someone intended to
explicitly enumerate all the possible cases and ensure that they were
correctly handled.

I have some other patches kicking around which will fold to REDUC_MAX_EXPR,
which will be another unhandled tree code. The approach of adding a case
in potential_constant_expression_1 each time a shiny new tree code appears
just feels wrong.

James
Gabriel Dos Reis - March 20, 2013, 7:08 p.m.
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.

-- Gaby
Jason Merrill - March 20, 2013, 7:25 p.m.
On 03/20/2013 02:03 PM, James Greenhalgh wrote:
> The argument would be; if we found an unexpected tree code then
> it must have been introduced by folding. If it was a constant,
> folding would have returned the constant not the expression. So it must
> not be a constant.

Unless the operands involve calls to constexpr functions, which aren't 
folded by fold.

Jason

Patch

diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
index 9446f83..232057a 100644
--- a/gcc/cp/semantics.c
+++ b/gcc/cp/semantics.c
@@ -8600,6 +8600,7 @@  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 UNARY_PLUS_EXPR:
       return potential_constant_expression_1 (TREE_OPERAND (t, 0), rval,
 					      flags);