| Message ID | 20151119062014.GA2345@jaguar.corp.atmel.com |
|---|---|
| State | New |
| Headers | show |
On 11/18/2015 11:20 PM, Senthil Kumar Selvaraj wrote: > On Wed, Nov 18, 2015 at 09:36:21AM +0100, Richard Biener wrote: >> >> Otherwise ok. > > See modified patch below. If you think vrp98.c is unnecessary, feel free > to dump it :). > > If ok, could you commit it for me please? I don't have commit access. > > Regards > Senthil > > gcc/ChangeLog > 2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > * tree.h (desired_pro_or_demotion_p): New function. > * tree-vrp.c (simplify_cond_using_ranges): Call it. > > gcc/testsuite/ChangeLog > 2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > * gcc.dg/tree-ssa/vrp98.c: New testcase. > * gcc.target/avr/uint8-single-reg.c: New testcase. I went ahead and committed this as-is. I do think the vrp98 testcase is useful as it verifies that VRP is doing what we want in a target independent way. It's a good complement to the AVR specific testcase. Thanks, Jeff
On Thu, Nov 19, 2015 at 10:31:41AM -0700, Jeff Law wrote: > On 11/18/2015 11:20 PM, Senthil Kumar Selvaraj wrote: > >On Wed, Nov 18, 2015 at 09:36:21AM +0100, Richard Biener wrote: > >> > >>Otherwise ok. > > > >See modified patch below. If you think vrp98.c is unnecessary, feel free > >to dump it :). > > > >If ok, could you commit it for me please? I don't have commit access. > > > >Regards > >Senthil > > > >gcc/ChangeLog > >2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > * tree.h (desired_pro_or_demotion_p): New function. > > * tree-vrp.c (simplify_cond_using_ranges): Call it. > > > >gcc/testsuite/ChangeLog > >2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > * gcc.dg/tree-ssa/vrp98.c: New testcase. > > * gcc.target/avr/uint8-single-reg.c: New testcase. > I went ahead and committed this as-is. > > I do think the vrp98 testcase is useful as it verifies that VRP is doing > what we want in a target independent way. It's a good complement to the AVR > specific testcase. I see the same problem on gcc-5-branch as well. Would it be ok to backport the fix to that branch as well? Regards Senthil > > Thanks, > Jeff >
On 11/20/2015 10:04 AM, Senthil Kumar Selvaraj wrote: > On Thu, Nov 19, 2015 at 10:31:41AM -0700, Jeff Law wrote: >> On 11/18/2015 11:20 PM, Senthil Kumar Selvaraj wrote: >>> On Wed, Nov 18, 2015 at 09:36:21AM +0100, Richard Biener wrote: >>>> >>>> Otherwise ok. >>> >>> See modified patch below. If you think vrp98.c is unnecessary, feel free >>> to dump it :). >>> >>> If ok, could you commit it for me please? I don't have commit access. >>> >>> Regards >>> Senthil >>> >>> gcc/ChangeLog >>> 2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> >>> >>> * tree.h (desired_pro_or_demotion_p): New function. >>> * tree-vrp.c (simplify_cond_using_ranges): Call it. >>> >>> gcc/testsuite/ChangeLog >>> 2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> >>> >>> * gcc.dg/tree-ssa/vrp98.c: New testcase. >>> * gcc.target/avr/uint8-single-reg.c: New testcase. >> I went ahead and committed this as-is. >> >> I do think the vrp98 testcase is useful as it verifies that VRP is doing >> what we want in a target independent way. It's a good complement to the AVR >> specific testcase. > > I see the same problem on gcc-5-branch as well. Would it be ok to > backport the fix to that branch as well? That's a call for the release managers. I typically don't backport anything expect ICE or incorrect code generation fixes as I tend to be very conservative on what goes onto a release branch. Jakub, Richi or Joseph would need to ack into a release branch. jeff
On Fri, 20 Nov 2015, Jeff Law wrote: > On 11/20/2015 10:04 AM, Senthil Kumar Selvaraj wrote: > > On Thu, Nov 19, 2015 at 10:31:41AM -0700, Jeff Law wrote: > > > On 11/18/2015 11:20 PM, Senthil Kumar Selvaraj wrote: > > > > On Wed, Nov 18, 2015 at 09:36:21AM +0100, Richard Biener wrote: > > > > > > > > > > Otherwise ok. > > > > > > > > See modified patch below. If you think vrp98.c is unnecessary, feel free > > > > to dump it :). > > > > > > > > If ok, could you commit it for me please? I don't have commit access. > > > > > > > > Regards > > > > Senthil > > > > > > > > gcc/ChangeLog > > > > 2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > > > > > * tree.h (desired_pro_or_demotion_p): New function. > > > > * tree-vrp.c (simplify_cond_using_ranges): Call it. > > > > > > > > gcc/testsuite/ChangeLog > > > > 2015-11-19 Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> > > > > > > > > * gcc.dg/tree-ssa/vrp98.c: New testcase. > > > > * gcc.target/avr/uint8-single-reg.c: New testcase. > > > I went ahead and committed this as-is. > > > > > > I do think the vrp98 testcase is useful as it verifies that VRP is doing > > > what we want in a target independent way. It's a good complement to the > > > AVR > > > specific testcase. > > > > I see the same problem on gcc-5-branch as well. Would it be ok to > > backport the fix to that branch as well? > That's a call for the release managers. I typically don't backport anything > expect ICE or incorrect code generation fixes as I tend to be very > conservative on what goes onto a release branch. > > Jakub, Richi or Joseph would need to ack into a release branch. As this is fixes a regression it qualifies in principle. But as it is an optimization regression only I'd prefer to wait a bit to look for fallout. Richard. > jeff > >
diff --git gcc/testsuite/gcc.dg/tree-ssa/vrp98.c gcc/testsuite/gcc.dg/tree-ssa/vrp98.c new file mode 100644 index 0000000..982f091 --- /dev/null +++ gcc/testsuite/gcc.dg/tree-ssa/vrp98.c @@ -0,0 +1,41 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target int128 } */ +/* { 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/testsuite/gcc.target/avr/uint8-single-reg.c gcc/testsuite/gcc.target/avr/uint8-single-reg.c new file mode 100644 index 0000000..291b56c --- /dev/null +++ gcc/testsuite/gcc.target/avr/uint8-single-reg.c @@ -0,0 +1,24 @@ +/* { dg-do compile } */ +/* { dg-options "-Os" } */ + +/* This testcase verifies that a uint8_t variable assigned from a wider variable + with the same range is held in a single register. VRP must not fold away the + conversion and use two regs to hold the uint16_t - widenings are ok only upto + word mode (1 byte for AVR). +*/ + +unsigned int foo(const unsigned int wvalue) +{ + const unsigned char type = (wvalue >> 8); + unsigned int size = 0; + + if (type == 1) + { + size = 20; + } + return size; +} + +/* { dg-final { scan-assembler "cpi r25,lo8\\(1\\)" } } */ +/* { dg-final { scan-assembler-not "cpc r\\d+,__zero_reg__" } } */ + diff --git gcc/tree-vrp.c gcc/tree-vrp.c index e67048e..6a4ff30 100644 --- gcc/tree-vrp.c +++ gcc/tree-vrp.c @@ -9459,7 +9459,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 (TREE_TYPE (innerop), TREE_TYPE (op0))) { value_range *vr = get_value_range (innerop); diff --git gcc/tree.h gcc/tree.h index 41c0f7c..cb52deb 100644 --- gcc/tree.h +++ gcc/tree.h @@ -5358,4 +5358,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 (const_tree to_type, const_tree from_type) +{ + unsigned int to_type_precision = TYPE_PRECISION (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 (from_type); +} + #endif /* GCC_TREE_H */