From: Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
To: Richard Biener <rguenther@suse.de>
Cc: <gcc-patches@gcc.gnu.org>, <law@redhat.com>
Subject: Re: [Patch, vrp] Allow VRP type conversion folding only for widenings upto word mode
Date: Tue, 17 Nov 2015 19:53:00 -0000 [thread overview]
Message-ID: <20151117195241.GA5579@jaguar.atmel.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1511160956220.4884@t29.fhfr.qr>
On Mon, Nov 16, 2015 at 10:02:15AM +0100, Richard Biener wrote:
> On Sat, 14 Nov 2015, Senthil Kumar Selvaraj wrote:
>
> > On Sat, Nov 14, 2015 at 09:57:40AM +0100, Richard Biener wrote:
> > > On November 14, 2015 9:49:28 AM GMT+01:00, Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> wrote:
> > > >On Sat, Nov 14, 2015 at 09:13:41AM +0100, Marc Glisse wrote:
> > > >> On Sat, 14 Nov 2015, Senthil Kumar Selvaraj wrote:
> > > >>
> > > >> >This patch came out of a discussion held in the gcc mailing list
> > > >> >(https://gcc.gnu.org/ml/gcc/2015-11/msg00067.html).
> > > >> >
> > > >> >The patch restricts folding of conditional exprs with lhs previously
> > > >> >set by a type conversion to occur only if the source of the type
> > > >> >conversion's mode is word mode or smaller.
> > > >> >
> > > >> >Bootstrapped and reg tested on x86_64 (with
> > > >--enable-languages=c,c++).
> > > >> >
> > > >> >If ok, could you commit please? I don't have commit access.
> > > >> >
> > > >> >Regards
> > > >> >Senthil
> > > >> >
> > > >> >gcc/ChangeLog
> > > >> >
> > > >> >2015-11-11 Senthil Kumar Selvaraj
> > > ><senthil_kumar.selvaraj@atmel.com>
> > > >> >
> > > >> > * tree-vrp.c (simplify_cond_using_ranges): Fold only
> > > >> > if innerop's mode is word_mode or smaller.
> > > >> >
> > > >> >
> > > >> >diff --git gcc/tree-vrp.c gcc/tree-vrp.c
> > > >> >index e2393e4..c139bc6 100644
> > > >> >--- gcc/tree-vrp.c
> > > >> >+++ gcc/tree-vrp.c
> > > >> >@@ -9467,6 +9467,8 @@ simplify_cond_using_ranges (gcond *stmt)
> > > >> > innerop = gimple_assign_rhs1 (def_stmt);
> > > >> >
> > > >> > if (TREE_CODE (innerop) == SSA_NAME
> > > >> >+ && (GET_MODE_SIZE(TYPE_MODE(TREE_TYPE(innerop)))
> > > >> >+ <= GET_MODE_SIZE(word_mode))
> > > >> > && !POINTER_TYPE_P (TREE_TYPE (innerop)))
> > > >> > {
> > > >> > value_range *vr = get_value_range (innerop);
> > > >>
> > > >> I thought the result of the discussion was that the transformation is
> > > >ok if
> > > >> either it is narrowing or it widens but to something no bigger than
> > > >> word_mode. So you should have 2 comparisons, or 1 with a max.
> > > >
> > > >Hmm, I came to the opposite conclusion - I thought Richard only okayed
> > > >"widening upto word-mode", not the narrowing.
> > >
> > > I didn't mean to suggest narrowing is not OK. In fact narrowing is always OK.
> >
> > My bad. Here's a revised patch that checks for both conditions, using
> > max as Marc suggested to limit to word_mode or narrowing conversions.
> >
> > Bootstrapped and regtested for x86_64 with c and c++.
> >
> > Is this ok? If yes, would you commit it
> > for me please? I don't have commit access.
> >
> > gcc/ChangeLog
> > 2015-11-14 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com>
> >
> > * tree-vrp.c (simplify_cond_using_ranges): Fold only
> > if innerop's mode smaller or equal to word_mode or op0's mode.
> >
> >
> > diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> > index e2393e4..cfd90e7 100644
> > --- a/gcc/tree-vrp.c
> > +++ b/gcc/tree-vrp.c
> > @@ -9467,7 +9467,10 @@ simplify_cond_using_ranges (gcond *stmt)
> > innerop = gimple_assign_rhs1 (def_stmt);
> >
> > if (TREE_CODE (innerop) == SSA_NAME
> > - && !POINTER_TYPE_P (TREE_TYPE (innerop)))
> > + && !POINTER_TYPE_P (TREE_TYPE (innerop))
> > + && (GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (innerop)))
> > + <= std::max (GET_MODE_SIZE (word_mode),
> > + GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (op0))))))
>
> Please use TYPE_PRECISION (...) and GET_MODE_PRECISION (word_mode) and
> add a comment as to what we are testing here and why.
>
> Btw, ideally we'd factor out a
>
> bool
> desired_pro_or_demotion_p (tree to_type, tree from_type) {}
>
> function somewhere as we have similar tests throughout the compiler
> that we might want to unify (and also have a central place to
> eventually add a target hook if ever desired).
>
> In fact in other places we also check that the type we promote/demote
> to matches its mode precision or the type we promote/demote from
> already does not.
>
> I'd suggest tree.[ch] for that function.
>
> Please also add a testcase.
How does the below patch look? Bootstrapped, but not regtested yet.
The testcase was rather tricky to write - I wasn't sure how to reliably
get a type bigger than a word for all targets. I resorted to __int128,
not sure it's a good idea though - I should probably add dg-skip-if for
targets that don't support that. Do you know of a better way to write
that?
Regards
Senthil
diff --git gcc/testsuite/gcc.dg/tree-ssa/vrp98.c gcc/testsuite/gcc.dg/tree-ssa/vrp98.c
new file mode 100644
index 0000000..448ceba
--- /dev/null
+++ gcc/testsuite/gcc.dg/tree-ssa/vrp98.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fdump-tree-vrp1-details" } */
+
+#include <stdint.h>
+#include <limits.h>
+
+typedef unsigned int word __attribute__((mode(word)));
+typedef unsigned __int128 bigger_than_word;
+
+int
+foo (bigger_than_word a, word b, uint8_t c)
+{
+ /* Must fold use of t1 into use of b, as b is no wider than word_mode. */
+ const uint8_t t1 = b % UCHAR_MAX;
+
+ /* Must NOT fold use of t2 into use of a, as a is wider than word_mode. */
+ const uint8_t t2 = a % UCHAR_MAX;
+
+ /* Must fold use of t3 into use of c, as c is narrower than t3. */
+ const uint32_t t3 = (const uint32_t)(c >> 1);
+
+ uint16_t ret = 0;
+
+ if (t1 == 1)
+ ret = 20;
+ else if (t2 == 2)
+ ret = 30;
+ else if (t3 == 3)
+ ret = 40;
+ /* Th extra condition below is necessary to prevent a prior pass from
+ folding away the cast. Ignored in scan-tree-dump. */
+ else if (t3 == 4)
+ ret = 50;
+
+ return ret;
+}
+
+/* { dg-final { scan-tree-dump "Folded into: if \\(_\[0-9\]+ == 1\\)" "vrp1" } } */
+/* { dg-final { scan-tree-dump-not "Folded into: if \\(_\[0-9\]+ == 2\\)" "vrp1" } } */
+/* { dg-final { scan-tree-dump "Folded into: if \\(_\[0-9\]+ == 3\\)" "vrp1" } } */
diff --git gcc/tree-vrp.c gcc/tree-vrp.c
index 5d085b4..4513b88 100644
--- gcc/tree-vrp.c
+++ gcc/tree-vrp.c
@@ -9467,7 +9467,8 @@ simplify_cond_using_ranges (gcond *stmt)
innerop = gimple_assign_rhs1 (def_stmt);
if (TREE_CODE (innerop) == SSA_NAME
- && !POINTER_TYPE_P (TREE_TYPE (innerop)))
+ && !POINTER_TYPE_P (TREE_TYPE (innerop))
+ && desired_pro_or_demotion_p (innerop, op0))
{
value_range *vr = get_value_range (innerop);
diff --git gcc/tree.h gcc/tree.h
index 0b9c3b9..91099ed 100644
--- gcc/tree.h
+++ gcc/tree.h
@@ -5316,4 +5316,18 @@ get_decl_source_range (tree decl)
return get_range_from_loc (line_table, loc);
}
+/* Return true if it makes sense to promote/demote from_type to to_type. */
+inline bool
+desired_pro_or_demotion_p (tree to_type, tree from_type)
+{
+ unsigned int to_type_precision = TYPE_PRECISION (TREE_TYPE (to_type));
+
+ /* OK to promote if to_type is no bigger than word_mode. */
+ if (to_type_precision <= GET_MODE_PRECISION (word_mode))
+ return true;
+
+ /* Otherwise, allow only if narrowing or same precision conversions. */
+ return to_type_precision <= TYPE_PRECISION (TREE_TYPE (from_type));
+}
+
#endif /* GCC_TREE_H */
>
> Thanks,
> Richard.
>
> > {
> > value_range *vr = get_value_range (innerop);
> >
> >
> > Regards
> > Senthil
> > >
> > > Richard.
> > >
> > > >Richard?
> > > >
> > > >Regards
> > > >Senthil
> > > >>
> > > >> --
> > > >> Marc Glisse
> > >
> > >
> >
> >
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
next prev parent reply other threads:[~2015-11-17 19:53 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-14 7:12 Senthil Kumar Selvaraj
2015-11-14 8:13 ` Marc Glisse
2015-11-14 8:50 ` Senthil Kumar Selvaraj
2015-11-14 8:57 ` Richard Biener
2015-11-14 18:11 ` Senthil Kumar Selvaraj
2015-11-16 9:02 ` Richard Biener
2015-11-17 19:53 ` Senthil Kumar Selvaraj [this message]
2015-11-18 8:36 ` Richard Biener
2015-11-19 6:20 ` Senthil Kumar Selvaraj
2015-11-19 17:31 ` Jeff Law
2015-11-20 17:04 ` Senthil Kumar Selvaraj
2015-11-20 17:34 ` Jeff Law
2015-11-23 9:54 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151117195241.GA5579@jaguar.atmel.com \
--to=senthil_kumar.selvaraj@atmel.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).