[PATCH] Fix c/69643, named address space wrong-code

[Richard Biener]

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
>