| Message ID | 56B274D2.8020109@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 3, 2016 at 10:44 PM, Richard Henderson <rth@redhat.com> wrote: > On 02/04/2016 07:30 AM, Richard Henderson wrote: >> >> On 02/04/2016 12:46 AM, Richard Biener wrote: >>> >>> As for a patch I'd repeatedly pondered on not stripping int <-> pointer >>> conversions at all, similar to what STRIP_SIGN_NOPS does. Don't remember >>> actually trying this or the fallout though. >> >> >> I'll run that through a test cycle and see what happens. > > > > +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat scan-tree-dump-times > original "& 15" 1 > +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat scan-tree-dump-times > original "return [^\\n0-9]*0;" 2 > +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat scan-tree-dump-times > original "return [^\\n0-9]*12;" 1 > +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "&c4 & 3" 0 > +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "&c8 & 3" 0 > +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "return 0" 2 > +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "& 3" 0 > +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 0" 1 > +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 1" 1 > +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 2" 1 > +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 3" 1 > +FAIL: gcc.dg/fold-bitand-3.c scan-tree-dump-times original "& 3" 0 > +FAIL: gcc.dg/fold-bitand-3.c scan-tree-dump-times original "return 1" 2 > +FAIL: gcc.dg/pr52355.c (test for excess errors) > +FAIL: gcc.dg/tree-ssa/foldaddr-1.c scan-tree-dump-times original "return 0" > 1 > +FAIL: gcc.dg/tree-ssa/ivopt_4.c scan-tree-dump-times ivopts "ivtmp.[0-9_]* > = PHI <" 1 > +FAIL: gcc.dg/tree-ssa/pr21985.c scan-tree-dump-times optimized "foo > \\\\([0-9]*\\\\)" 2 > +FAIL: gcc.dg/tree-ssa/pr22051-2.c scan-tree-dump-times optimized "r_. = > \\\\(int\\\\) q" 1 > +FAIL: gcc.target/i386/addr-space-5.c scan-assembler gs: > > > So, it even fails the new test I added there at the end. > Patch below, just in case I've misunderstood what you suggested. Yes, that's what I had suggested. Of course to fix the addr-space issue it has to add the certainly missing addr-space compatibility fix for the pointer-pointer cast case. So yes, somewhat what I expected, some missed fold opportunities which expect the pointer-int cast stripping. I would guess that in match.pd /* Two conversions in a row are not needed unless: - some conversion is floating-point (overstrict for now), or - some conversion is a vector (overstrict for now), or - the intermediate type is narrower than both initial and final, or - the intermediate type and innermost type differ in signedness, and the outermost type is wider than the intermediate, or - the initial type is a pointer type and the precisions of the intermediate and final types differ, or - the final type is a pointer type and the precisions of the initial and intermediate types differ. */ (if (! inside_float && ! inter_float && ! final_float && ! inside_vec && ! inter_vec && ! final_vec && (inter_prec >= inside_prec || inter_prec >= final_prec) && ! (inside_int && inter_int && inter_unsignedp != inside_unsignedp && inter_prec < final_prec) && ((inter_unsignedp && inter_prec > inside_prec) == (final_unsignedp && final_prec > inter_prec)) && ! (inside_ptr && inter_prec != final_prec) && ! (final_ptr && inside_prec != inter_prec) && ! (final_prec != GET_MODE_PRECISION (TYPE_MODE (type)) && TYPE_MODE (type) == TYPE_MODE (inter_type))) (ocvt @0)) also needs adjustments. That's to avoid (ptr-w/addr-spaceA *)(uintptr_t)ptr-w/addr-spaceB which would strip the integer conversion and thus would require a ADDR_SPACE_CONVERT_EXPR (if the addr-spaces are related) or it would be even bogus. Now looking at your original patch + /* Do not strip casts into or out of differing address spaces. */ + if (POINTER_TYPE_P (outer_type) + && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) != ADDR_SPACE_GENERIC) + { + if (!POINTER_TYPE_P (inner_type) + || (TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) + != TYPE_ADDR_SPACE (TREE_TYPE (inner_type)))) + return false; + } + else if (POINTER_TYPE_P (inner_type) + && TYPE_ADDR_SPACE (TREE_TYPE (inner_type)) != ADDR_SPACE_GENERIC) + { + /* We already know that outer_type is not a pointer with + a non-generic address space. */ + return false; + } it really looks like sth is wrong with our IL if such complications are necessary here ... Thus I'd prefer to at least re-write it as /* Do not strip casts changing the address space to/from non-ADDR_SPACE_GENERIC. */ if ((POINTER_TYPE_P (outer_type) && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) != ADDR_SPACE_GENERIC) || (POINTER_TYPE_P (inner_type) && TYPE_ADDR_SPACE (TREE_TYPE (inner_type)) != ADDR_SPACE_GENERIC)) return (POINTER_TYPE_P (outer_type) && POINTER_TYPE (inner_type) && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) == TYPE_ADDR_SPACE (TREE_TYPE (inner_type))); with the goal to get rid of special-casing ADDR_SPACE_GENERIC in GCC 7 (and thus analyzing/fixing the folding regression). The above will end up failing to strip the nop conversions in (ptr *)(uintptr_t)p if both ptr and p have a non-generic address-space. Of course we now expect folding to deal with conversion sequences and eventually STRIP_NOPS and friends should be changed to no longer loop (again, don't remember trying or any actual fallout in doing that). GCC 7 again... Thanks, Richard. > > > r~ > > > > diff --git a/gcc/tree.c b/gcc/tree.c > index fa7646b..3e79c4b 100644 > --- a/gcc/tree.c > +++ b/gcc/tree.c > @@ -12219,6 +12219,10 @@ block_ultimate_origin (const_tree block) > bool > tree_nop_conversion_p (const_tree outer_type, const_tree inner_type) > { > + /* Do not strip conversions between pointers and integers. */ > + if (POINTER_TYPE_P (outer_type) != POINTER_TYPE_P (inner_type)) > + return false; > + > /* Use precision rather then machine mode when we can, which gives > the correct answer even for submode (bit-field) types. */ > if ((INTEGRAL_TYPE_P (outer_type) > @@ -12272,8 +12276,7 @@ tree_sign_nop_conversion (const_tree exp) > outer_type = TREE_TYPE (exp); > inner_type = TREE_TYPE (TREE_OPERAND (exp, 0)); > > - return (TYPE_UNSIGNED (outer_type) == TYPE_UNSIGNED (inner_type) > - && POINTER_TYPE_P (outer_type) == POINTER_TYPE_P (inner_type)); > + return TYPE_UNSIGNED (outer_type) == TYPE_UNSIGNED (inner_type); > } > > /* Strip conversions from EXP according to tree_nop_conversion and >
On 02/04/2016 10:07 PM, Richard Biener wrote: > On Wed, Feb 3, 2016 at 10:44 PM, Richard Henderson <rth@redhat.com> wrote: >> On 02/04/2016 07:30 AM, Richard Henderson wrote: >>> >>> On 02/04/2016 12:46 AM, Richard Biener wrote: >>>> >>>> As for a patch I'd repeatedly pondered on not stripping int <-> pointer >>>> conversions at all, similar to what STRIP_SIGN_NOPS does. Don't remember >>>> actually trying this or the fallout though. >>> >>> >>> I'll run that through a test cycle and see what happens. >> >> >> >> +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat scan-tree-dump-times >> original "& 15" 1 >> +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat scan-tree-dump-times >> original "return [^\\n0-9]*0;" 2 >> +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat scan-tree-dump-times >> original "return [^\\n0-9]*12;" 1 >> +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "&c4 & 3" 0 >> +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "&c8 & 3" 0 >> +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "return 0" 2 >> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "& 3" 0 >> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 0" 1 >> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 1" 1 >> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 2" 1 >> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return 3" 1 >> +FAIL: gcc.dg/fold-bitand-3.c scan-tree-dump-times original "& 3" 0 >> +FAIL: gcc.dg/fold-bitand-3.c scan-tree-dump-times original "return 1" 2 >> +FAIL: gcc.dg/pr52355.c (test for excess errors) >> +FAIL: gcc.dg/tree-ssa/foldaddr-1.c scan-tree-dump-times original "return 0" >> 1 >> +FAIL: gcc.dg/tree-ssa/ivopt_4.c scan-tree-dump-times ivopts "ivtmp.[0-9_]* >> = PHI <" 1 >> +FAIL: gcc.dg/tree-ssa/pr21985.c scan-tree-dump-times optimized "foo >> \\\\([0-9]*\\\\)" 2 >> +FAIL: gcc.dg/tree-ssa/pr22051-2.c scan-tree-dump-times optimized "r_. = >> \\\\(int\\\\) q" 1 >> +FAIL: gcc.target/i386/addr-space-5.c scan-assembler gs: >> >> >> So, it even fails the new test I added there at the end. >> Patch below, just in case I've misunderstood what you suggested. > > Yes, that's what I had suggested. Of course to fix the addr-space issue > it has to add the certainly missing addr-space compatibility fix for > the pointer-pointer cast case. So yes, somewhat what I expected, some > missed fold opportunities which expect the pointer-int cast stripping. > > > I would guess that in match.pd > > /* Two conversions in a row are not needed unless: > - some conversion is floating-point (overstrict for now), or > - some conversion is a vector (overstrict for now), or > - the intermediate type is narrower than both initial and > final, or > - the intermediate type and innermost type differ in signedness, > and the outermost type is wider than the intermediate, or > - the initial type is a pointer type and the precisions of the > intermediate and final types differ, or > - the final type is a pointer type and the precisions of the > initial and intermediate types differ. */ > (if (! inside_float && ! inter_float && ! final_float > && ! inside_vec && ! inter_vec && ! final_vec > && (inter_prec >= inside_prec || inter_prec >= final_prec) > && ! (inside_int && inter_int > && inter_unsignedp != inside_unsignedp > && inter_prec < final_prec) > && ((inter_unsignedp && inter_prec > inside_prec) > == (final_unsignedp && final_prec > inter_prec)) > && ! (inside_ptr && inter_prec != final_prec) > && ! (final_ptr && inside_prec != inter_prec) > && ! (final_prec != GET_MODE_PRECISION (TYPE_MODE (type)) > && TYPE_MODE (type) == TYPE_MODE (inter_type))) > (ocvt @0)) > > also needs adjustments. That's to avoid (ptr-w/addr-spaceA > *)(uintptr_t)ptr-w/addr-spaceB > which would strip the integer conversion and thus would require a > ADDR_SPACE_CONVERT_EXPR > (if the addr-spaces are related) or it would be even bogus. > > Now looking at your original patch > > + /* Do not strip casts into or out of differing address spaces. */ > + if (POINTER_TYPE_P (outer_type) > + && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) != ADDR_SPACE_GENERIC) > + { > + if (!POINTER_TYPE_P (inner_type) > + || (TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) > + != TYPE_ADDR_SPACE (TREE_TYPE (inner_type)))) > + return false; > + } > + else if (POINTER_TYPE_P (inner_type) > + && TYPE_ADDR_SPACE (TREE_TYPE (inner_type)) != ADDR_SPACE_GENERIC) > + { > + /* We already know that outer_type is not a pointer with > + a non-generic address space. */ > + return false; > + } > > it really looks like sth is wrong with our IL if such complications > are necessary here ... > > Thus I'd prefer to at least re-write it as > > /* Do not strip casts changing the address space to/from > non-ADDR_SPACE_GENERIC. */ > if ((POINTER_TYPE_P (outer_type) && TYPE_ADDR_SPACE (TREE_TYPE > (outer_type)) != ADDR_SPACE_GENERIC) > || (POINTER_TYPE_P (inner_type) && TYPE_ADDR_SPACE (TREE_TYPE > (inner_type)) != ADDR_SPACE_GENERIC)) > return (POINTER_TYPE_P (outer_type) && POINTER_TYPE (inner_type) > && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) == > TYPE_ADDR_SPACE (TREE_TYPE (inner_type))); This version fails to fall through to the next code block when (1) Both types are pointers, (2) Both types have the same address space, which will do the wrong thing when (3) The pointers have different modes. Recall that several ports allow multiple modes for pointers. > with the goal to get rid of special-casing ADDR_SPACE_GENERIC in GCC 7 > (and thus analyzing/fixing the folding regression). Sure. r~
On February 4, 2016 10:04:47 PM GMT+01:00, Richard Henderson <rth@redhat.com> wrote: >On 02/04/2016 10:07 PM, Richard Biener wrote: >> On Wed, Feb 3, 2016 at 10:44 PM, Richard Henderson <rth@redhat.com> >wrote: >>> On 02/04/2016 07:30 AM, Richard Henderson wrote: >>>> >>>> On 02/04/2016 12:46 AM, Richard Biener wrote: >>>>> >>>>> As for a patch I'd repeatedly pondered on not stripping int <-> >pointer >>>>> conversions at all, similar to what STRIP_SIGN_NOPS does. Don't >remember >>>>> actually trying this or the fallout though. >>>> >>>> >>>> I'll run that through a test cycle and see what happens. >>> >>> >>> >>> +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat >scan-tree-dump-times >>> original "& 15" 1 >>> +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat >scan-tree-dump-times >>> original "return [^\\n0-9]*0;" 2 >>> +FAIL: c-c++-common/fold-bitand-4.c -Wc++-compat >scan-tree-dump-times >>> original "return [^\\n0-9]*12;" 1 >>> +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "&c4 & >3" 0 >>> +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "&c8 & >3" 0 >>> +FAIL: gcc.dg/fold-bitand-1.c scan-tree-dump-times original "return >0" 2 >>> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "& 3" 0 >>> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return >0" 1 >>> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return >1" 1 >>> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return >2" 1 >>> +FAIL: gcc.dg/fold-bitand-2.c scan-tree-dump-times original "return >3" 1 >>> +FAIL: gcc.dg/fold-bitand-3.c scan-tree-dump-times original "& 3" 0 >>> +FAIL: gcc.dg/fold-bitand-3.c scan-tree-dump-times original "return >1" 2 >>> +FAIL: gcc.dg/pr52355.c (test for excess errors) >>> +FAIL: gcc.dg/tree-ssa/foldaddr-1.c scan-tree-dump-times original >"return 0" >>> 1 >>> +FAIL: gcc.dg/tree-ssa/ivopt_4.c scan-tree-dump-times ivopts >"ivtmp.[0-9_]* >>> = PHI <" 1 >>> +FAIL: gcc.dg/tree-ssa/pr21985.c scan-tree-dump-times optimized "foo >>> \\\\([0-9]*\\\\)" 2 >>> +FAIL: gcc.dg/tree-ssa/pr22051-2.c scan-tree-dump-times optimized >"r_. = >>> \\\\(int\\\\) q" 1 >>> +FAIL: gcc.target/i386/addr-space-5.c scan-assembler gs: >>> >>> >>> So, it even fails the new test I added there at the end. >>> Patch below, just in case I've misunderstood what you suggested. >> >> Yes, that's what I had suggested. Of course to fix the addr-space >issue >> it has to add the certainly missing addr-space compatibility fix for >> the pointer-pointer cast case. So yes, somewhat what I expected, >some >> missed fold opportunities which expect the pointer-int cast >stripping. >> >> >> I would guess that in match.pd >> >> /* Two conversions in a row are not needed unless: >> - some conversion is floating-point (overstrict for now), or >> - some conversion is a vector (overstrict for now), or >> - the intermediate type is narrower than both initial and >> final, or >> - the intermediate type and innermost type differ in >signedness, >> and the outermost type is wider than the intermediate, or >> - the initial type is a pointer type and the precisions of >the >> intermediate and final types differ, or >> - the final type is a pointer type and the precisions of the >> initial and intermediate types differ. */ >> (if (! inside_float && ! inter_float && ! final_float >> && ! inside_vec && ! inter_vec && ! final_vec >> && (inter_prec >= inside_prec || inter_prec >= final_prec) >> && ! (inside_int && inter_int >> && inter_unsignedp != inside_unsignedp >> && inter_prec < final_prec) >> && ((inter_unsignedp && inter_prec > inside_prec) >> == (final_unsignedp && final_prec > inter_prec)) >> && ! (inside_ptr && inter_prec != final_prec) >> && ! (final_ptr && inside_prec != inter_prec) >> && ! (final_prec != GET_MODE_PRECISION (TYPE_MODE (type)) >> && TYPE_MODE (type) == TYPE_MODE (inter_type))) >> (ocvt @0)) >> >> also needs adjustments. That's to avoid (ptr-w/addr-spaceA >> *)(uintptr_t)ptr-w/addr-spaceB >> which would strip the integer conversion and thus would require a >> ADDR_SPACE_CONVERT_EXPR >> (if the addr-spaces are related) or it would be even bogus. >> >> Now looking at your original patch >> >> + /* Do not strip casts into or out of differing address spaces. */ >> + if (POINTER_TYPE_P (outer_type) >> + && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) != >ADDR_SPACE_GENERIC) >> + { >> + if (!POINTER_TYPE_P (inner_type) >> + || (TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) >> + != TYPE_ADDR_SPACE (TREE_TYPE (inner_type)))) >> + return false; >> + } >> + else if (POINTER_TYPE_P (inner_type) >> + && TYPE_ADDR_SPACE (TREE_TYPE (inner_type)) != >ADDR_SPACE_GENERIC) >> + { >> + /* We already know that outer_type is not a pointer with >> + a non-generic address space. */ >> + return false; >> + } >> >> it really looks like sth is wrong with our IL if such complications >> are necessary here ... >> >> Thus I'd prefer to at least re-write it as >> >> /* Do not strip casts changing the address space to/from >> non-ADDR_SPACE_GENERIC. */ >> if ((POINTER_TYPE_P (outer_type) && TYPE_ADDR_SPACE (TREE_TYPE >> (outer_type)) != ADDR_SPACE_GENERIC) >> || (POINTER_TYPE_P (inner_type) && TYPE_ADDR_SPACE (TREE_TYPE >> (inner_type)) != ADDR_SPACE_GENERIC)) >> return (POINTER_TYPE_P (outer_type) && POINTER_TYPE (inner_type) >> && TYPE_ADDR_SPACE (TREE_TYPE (outer_type)) == >> TYPE_ADDR_SPACE (TREE_TYPE (inner_type))); > >This version fails to fall through to the next code block when > (1) Both types are pointers, > (2) Both types have the same address space, >which will do the wrong thing when > (3) The pointers have different modes. > >Recall that several ports allow multiple modes for pointers. Oh, I thought they would be different address spaces. So we'd need to add a mode check as well. I hope we don't have different type bit-precision with the same mode for pointers here? Richard. >> with the goal to get rid of special-casing ADDR_SPACE_GENERIC in GCC >7 >> (and thus analyzing/fixing the folding regression). > >Sure. > > >r~
On 02/05/2016 08:59 AM, Richard Biener wrote: >> This version fails to fall through to the next code block when >> (1) Both types are pointers, >> (2) Both types have the same address space, >> which will do the wrong thing when >> (3) The pointers have different modes. >> >> Recall that several ports allow multiple modes for pointers. > > Oh, I thought they would be different address spaces. They probably should be. > So we'd need to add a mode check as well. Yes. But why make this one expression so complicated that it's hard to read, as opposed to letting the existing code that checks modes check the mode? > I hope we don't have different type bit-precision with the same mode for pointers here? Not that I'm aware. ;-) r~
On Thu, Feb 4, 2016 at 11:35 PM, Richard Henderson <rth@redhat.com> wrote: > On 02/05/2016 08:59 AM, Richard Biener wrote: >>> >>> This version fails to fall through to the next code block when >>> (1) Both types are pointers, >>> (2) Both types have the same address space, >>> which will do the wrong thing when >>> (3) The pointers have different modes. >>> >>> Recall that several ports allow multiple modes for pointers. >> >> >> Oh, I thought they would be different address spaces. > > > They probably should be. > >> So we'd need to add a mode check as well. > > > Yes. But why make this one expression so complicated that it's hard to > read, > as opposed to letting the existing code that checks modes check the mode? Works for me. >> I hope we don't have different type bit-precision with the same mode for >> pointers here? > > > Not that I'm aware. ;-) > > > r~
diff --git a/gcc/tree.c b/gcc/tree.c index fa7646b..3e79c4b 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -12219,6 +12219,10 @@ block_ultimate_origin (const_tree block) bool tree_nop_conversion_p (const_tree outer_type, const_tree inner_type) { + /* Do not strip conversions between pointers and integers. */ + if (POINTER_TYPE_P (outer_type) != POINTER_TYPE_P (inner_type)) + return false; + /* Use precision rather then machine mode when we can, which gives the correct answer even for submode (bit-field) types. */ if ((INTEGRAL_TYPE_P (outer_type) @@ -12272,8 +12276,7 @@ tree_sign_nop_conversion (const_tree exp) outer_type = TREE_TYPE (exp); inner_type = TREE_TYPE (TREE_OPERAND (exp, 0)); - return (TYPE_UNSIGNED (outer_type) == TYPE_UNSIGNED (inner_type) - && POINTER_TYPE_P (outer_type) == POINTER_TYPE_P (inner_type)); + return TYPE_UNSIGNED (outer_type) == TYPE_UNSIGNED (inner_type); } /* Strip conversions from EXP according to tree_nop_conversion and