* [c++-delayed-folding] First stab at convert_to_integer
@ 2015-10-16 17:42 Marek Polacek
2015-10-17 0:25 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2015-10-16 17:42 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
I felt like it'd be good to resolve some unfinished business in
convert_to_integer before starting messing with other convert_to_*
functions. This is a response to
<https://gcc.gnu.org/ml/gcc-patches/2015-09/msg01255.html>.
> > + if (!dofold)
> > + {
> > + expr = build1 (CONVERT_EXPR,
> > + lang_hooks.types.type_for_size
> > + (TYPE_PRECISION (intype), 0),
> > + expr);
> > + return build1 (CONVERT_EXPR, type, expr);
> > + }
>
> When we're not folding, I don't think we want to do the two-step
> conversion, just the second one. And we might want to use NOP_EXPR
> instead of CONVERT_EXPR, but I'm not sure about that.
Changed. I kept CONVERT_EXPR there even though NOP_EXPR seemed to work as
well.
> > @@ -818,10 +828,15 @@ convert_to_integer (tree type, tree expr)
> > if (TYPE_UNSIGNED (typex))
> > typex = signed_type_for (typex);
> > }
> > - return convert (type,
> > - fold_build2 (ex_form, typex,
> > - convert (typex, arg0),
> > - convert (typex, arg1)));
> > + if (dofold)
> > + return convert (type,
> > + fold_build2 (ex_form, typex,
> > + convert (typex, arg0),
> > + convert (typex, arg1)));
> > + arg0 = build1 (CONVERT_EXPR, typex, arg0);
> > + arg1 = build1 (CONVERT_EXPR, typex, arg1);
> > + expr = build2 (ex_form, typex, arg0, arg1);
> > + return build1 (CONVERT_EXPR, type, expr);
>
> This code path seems to be for pushing a conversion down into a binary
> expression. We shouldn't do this at all when we aren't folding.
I tend to agree, but this case is tricky. What's this code about is
e.g. for
int
fn (long p, long o)
{
return p + o;
}
we want to narrow the operation and do the addition on unsigned ints and then
convert to int. We do it here because we're still missing the
promotion/demotion pass on GIMPLE (PR45397 / PR47477). Disabling this
optimization here would regress a few testcases, so I kept the code as it was.
Thoughts?
> > @@ -845,9 +860,14 @@ convert_to_integer (tree type, tree expr)
> >
> > if (!TYPE_UNSIGNED (typex))
> > typex = unsigned_type_for (typex);
> > + if (!dofold)
> > + return build1 (CONVERT_EXPR, type,
> > + build1 (ex_form, typex,
> > + build1 (CONVERT_EXPR, typex,
> > + TREE_OPERAND (expr, 0))));
>
> Likewise.
Changed.
> > @@ -867,6 +887,14 @@ convert_to_integer (tree type, tree expr)
> > the conditional and never loses. A COND_EXPR may have a throw
> > as one operand, which then has void type. Just leave void
> > operands as they are. */
> > + if (!dofold)
> > + return build3 (COND_EXPR, type, TREE_OPERAND (expr, 0),
> > + VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 1)))
> > + ? TREE_OPERAND (expr, 1)
> > + : build1 (CONVERT_EXPR, type, TREE_OPERAND (expr, 1)),
> > + VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 2)))
> > + ? TREE_OPERAND (expr, 2)
> > + : build1 (CONVERT_EXPR, type, TREE_OPERAND (expr, 2)));
>
> Likewise.
Changed.
> > @@ -903,6 +933,10 @@ convert_to_integer (tree type, tree expr)
> > return build1 (FIXED_CONVERT_EXPR, type, expr);
> >
> > case COMPLEX_TYPE:
> > + if (!dofold)
> > + return build1 (CONVERT_EXPR, type,
> > + build1 (REALPART_EXPR,
> > + TREE_TYPE (TREE_TYPE (expr)), expr));
>
> Why can't we call convert here rather than build1 a CONVERT_EXPR?
I don't know if there was any particular reason but just build1 seems dubious
so I've changed this to convert and didn't see any problems.
> It would be good to ask a fold/convert maintainer to review the changes
> to this file, too.
Certainly; comments welcome.
Moreover, there are some places in the C++ FE where we still call
convert_to_integer and not convert_to_integer_nofold -- should they be
changed to the _nofold variant?
Bootstrapped/regtested on x86_64-linux, ok for branch?
diff --git gcc/convert.c gcc/convert.c
index fdb9b9a..40db767 100644
--- gcc/convert.c
+++ gcc/convert.c
@@ -571,13 +571,7 @@ convert_to_integer_1 (tree type, tree expr, bool dofold)
coexistence of multiple valid pointer sizes, so fetch the one we need
from the type. */
if (!dofold)
- {
- expr = build1 (CONVERT_EXPR,
- lang_hooks.types.type_for_size
- (TYPE_PRECISION (intype), 0),
- expr);
- return build1 (CONVERT_EXPR, type, expr);
- }
+ return build1 (CONVERT_EXPR, type, expr);
expr = fold_build1 (CONVERT_EXPR,
lang_hooks.types.type_for_size
(TYPE_PRECISION (intype), 0),
@@ -622,6 +616,8 @@ convert_to_integer_1 (tree type, tree expr, bool dofold)
else
code = NOP_EXPR;
+ if (!dofold)
+ return build1 (code, type, expr);
return fold_build1 (code, type, expr);
}
@@ -847,6 +843,9 @@ convert_to_integer_1 (tree type, tree expr, bool dofold)
/* This is not correct for ABS_EXPR,
since we must test the sign before truncation. */
{
+ if (!dofold)
+ break;
+
/* Do the arithmetic in type TYPEX,
then convert result to TYPE. */
tree typex = type;
@@ -860,11 +859,6 @@ convert_to_integer_1 (tree type, tree expr, bool dofold)
if (!TYPE_UNSIGNED (typex))
typex = unsigned_type_for (typex);
- if (!dofold)
- return build1 (CONVERT_EXPR, type,
- build1 (ex_form, typex,
- build1 (CONVERT_EXPR, typex,
- TREE_OPERAND (expr, 0))));
return convert (type,
fold_build1 (ex_form, typex,
convert (typex,
@@ -887,21 +881,15 @@ convert_to_integer_1 (tree type, tree expr, bool dofold)
the conditional and never loses. A COND_EXPR may have a throw
as one operand, which then has void type. Just leave void
operands as they are. */
- if (!dofold)
- return build3 (COND_EXPR, type, TREE_OPERAND (expr, 0),
+ if (dofold)
+ return
+ fold_build3 (COND_EXPR, type, TREE_OPERAND (expr, 0),
VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 1)))
? TREE_OPERAND (expr, 1)
- : build1 (CONVERT_EXPR, type, TREE_OPERAND (expr, 1)),
+ : convert (type, TREE_OPERAND (expr, 1)),
VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 2)))
? TREE_OPERAND (expr, 2)
- : build1 (CONVERT_EXPR, type, TREE_OPERAND (expr, 2)));
- return fold_build3 (COND_EXPR, type, TREE_OPERAND (expr, 0),
- VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 1)))
- ? TREE_OPERAND (expr, 1)
- : convert (type, TREE_OPERAND (expr, 1)),
- VOID_TYPE_P (TREE_TYPE (TREE_OPERAND (expr, 2)))
- ? TREE_OPERAND (expr, 2)
- : convert (type, TREE_OPERAND (expr, 2)));
+ : convert (type, TREE_OPERAND (expr, 2)));
default:
break;
@@ -934,9 +922,9 @@ convert_to_integer_1 (tree type, tree expr, bool dofold)
case COMPLEX_TYPE:
if (!dofold)
- return build1 (CONVERT_EXPR, type,
- build1 (REALPART_EXPR,
- TREE_TYPE (TREE_TYPE (expr)), expr));
+ return convert (type,
+ build1 (REALPART_EXPR,
+ TREE_TYPE (TREE_TYPE (expr)), expr));
return convert (type,
fold_build1 (REALPART_EXPR,
TREE_TYPE (TREE_TYPE (expr)), expr));
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [c++-delayed-folding] First stab at convert_to_integer
2015-10-16 17:42 [c++-delayed-folding] First stab at convert_to_integer Marek Polacek
@ 2015-10-17 0:25 ` Jason Merrill
2015-10-19 12:38 ` Marek Polacek
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2015-10-17 0:25 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 10/16/2015 07:35 AM, Marek Polacek wrote:
>> This code path seems to be for pushing a conversion down into a binary
>> expression. We shouldn't do this at all when we aren't folding.
>
> I tend to agree, but this case is tricky. What's this code about is
> e.g. for
>
> int
> fn (long p, long o)
> {
> return p + o;
> }
>
> we want to narrow the operation and do the addition on unsigned ints and then
> convert to int. We do it here because we're still missing the
> promotion/demotion pass on GIMPLE (PR45397 / PR47477). Disabling this
> optimization here would regress a few testcases, so I kept the code as it was.
> Thoughts?
That makes sense, but please add a comment referring to one of those PRs
and also add a note to the PR about this place. OK with that change.
> Moreover, there are some places in the C++ FE where we still call
> convert_to_integer and not convert_to_integer_nofold -- should they be
> changed to the _nofold variant?
Not in build_base_path; its arithmetic is compiler generated and should
really be delayed until genericize anyway. Likewise for
get_delta_difference.
I think the call in finish_omp_clauses could change.
Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [c++-delayed-folding] First stab at convert_to_integer
2015-10-17 0:25 ` Jason Merrill
@ 2015-10-19 12:38 ` Marek Polacek
2015-10-19 20:02 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2015-10-19 12:38 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Fri, Oct 16, 2015 at 02:07:51PM -1000, Jason Merrill wrote:
> On 10/16/2015 07:35 AM, Marek Polacek wrote:
> >>This code path seems to be for pushing a conversion down into a binary
> >>expression. We shouldn't do this at all when we aren't folding.
> >
> >I tend to agree, but this case is tricky. What's this code about is
> >e.g. for
> >
> >int
> >fn (long p, long o)
> >{
> > return p + o;
> >}
> >
> >we want to narrow the operation and do the addition on unsigned ints and then
> >convert to int. We do it here because we're still missing the
> >promotion/demotion pass on GIMPLE (PR45397 / PR47477). Disabling this
> >optimization here would regress a few testcases, so I kept the code as it was.
> >Thoughts?
>
> That makes sense, but please add a comment referring to one of those PRs and
> also add a note to the PR about this place. OK with that change.
Done. But I can't seem to commit the patch to the c++-delayed-folding
branch; is that somehow restricted? I'm getting:
svn: E170001: Commit failed (details follow):
svn: E170001: Authorization failed
svn: E170001: Your commit message was left in a temporary file:
svn: E170001: '/home/marek/svn/c++-delayed-folding/svn-commit.tmp'
and I've checked out the branch using
svn co svn://mpolacek@gcc.gnu.org/svn/gcc/branches/c++-delayed-folding/
> >Moreover, there are some places in the C++ FE where we still call
> >convert_to_integer and not convert_to_integer_nofold -- should they be
> >changed to the _nofold variant?
>
> Not in build_base_path; its arithmetic is compiler generated and should
> really be delayed until genericize anyway. Likewise for
> get_delta_difference.
>
> I think the call in finish_omp_clauses could change.
All right, I'll submit a separate patch. Thanks,
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [c++-delayed-folding] First stab at convert_to_integer
2015-10-19 12:38 ` Marek Polacek
@ 2015-10-19 20:02 ` Jason Merrill
2015-10-19 20:23 ` Marek Polacek
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2015-10-19 20:02 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On 10/19/2015 02:31 AM, Marek Polacek wrote:
> On Fri, Oct 16, 2015 at 02:07:51PM -1000, Jason Merrill wrote:
>> On 10/16/2015 07:35 AM, Marek Polacek wrote:
>>>> This code path seems to be for pushing a conversion down into a binary
>>>> expression. We shouldn't do this at all when we aren't folding.
>>>
>>> I tend to agree, but this case is tricky. What's this code about is
>>> e.g. for
>>>
>>> int
>>> fn (long p, long o)
>>> {
>>> return p + o;
>>> }
>>>
>>> we want to narrow the operation and do the addition on unsigned ints and then
>>> convert to int. We do it here because we're still missing the
>>> promotion/demotion pass on GIMPLE (PR45397 / PR47477). Disabling this
>>> optimization here would regress a few testcases, so I kept the code as it was.
>>> Thoughts?
>>
>> That makes sense, but please add a comment referring to one of those PRs and
>> also add a note to the PR about this place. OK with that change.
>
> Done. But I can't seem to commit the patch to the c++-delayed-folding
> branch; is that somehow restricted? I'm getting:
>
> svn: E170001: Commit failed (details follow):
> svn: E170001: Authorization failed
> svn: E170001: Your commit message was left in a temporary file:
> svn: E170001: '/home/marek/svn/c++-delayed-folding/svn-commit.tmp'
>
> and I've checked out the branch using
> svn co svn://mpolacek@gcc.gnu.org/svn/gcc/branches/c++-delayed-folding/
You need to use svn+ssh:// rather than svn:// if you want to be able to
commit. From svnwrite.html:
It is also possible to convert an existing SVN tree to use SSH by using
svn switch --relocate:
svn switch --relocate svn://gcc.gnu.org/svn/gcc
svn+ssh://username@gcc.gnu.org/svn/gcc
Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [c++-delayed-folding] First stab at convert_to_integer
2015-10-19 20:02 ` Jason Merrill
@ 2015-10-19 20:23 ` Marek Polacek
0 siblings, 0 replies; 5+ messages in thread
From: Marek Polacek @ 2015-10-19 20:23 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Mon, Oct 19, 2015 at 09:59:03AM -1000, Jason Merrill wrote:
> >Done. But I can't seem to commit the patch to the c++-delayed-folding
> >branch; is that somehow restricted? I'm getting:
> >
> >svn: E170001: Commit failed (details follow):
> >svn: E170001: Authorization failed
> >svn: E170001: Your commit message was left in a temporary file:
> >svn: E170001: '/home/marek/svn/c++-delayed-folding/svn-commit.tmp'
> >
> >and I've checked out the branch using
> >svn co svn://mpolacek@gcc.gnu.org/svn/gcc/branches/c++-delayed-folding/
>
> You need to use svn+ssh:// rather than svn:// if you want to be able to
> commit. From svnwrite.html:
>
> It is also possible to convert an existing SVN tree to use SSH by using svn
> switch --relocate:
>
> svn switch --relocate svn://gcc.gnu.org/svn/gcc
> svn+ssh://username@gcc.gnu.org/svn/gcc
Oh my, thanks. Committed now.
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-10-19 20:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-16 17:42 [c++-delayed-folding] First stab at convert_to_integer Marek Polacek
2015-10-17 0:25 ` Jason Merrill
2015-10-19 12:38 ` Marek Polacek
2015-10-19 20:02 ` Jason Merrill
2015-10-19 20:23 ` Marek Polacek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).