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

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

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

In gimple_fold_indirect_ref, we STRIP_NOPS, find the ADDR_EXPR, and fold 
everything away.

I can't imagine it ever being correct to drop an address space change between 
pointers, so I've modified tree_nop_conversion_p.  Anything else seems to 
require more checks every places we use STRIP_NOPS.

Ok?


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 2015 bytes --]

diff --git a/gcc/testsuite/gcc.target/i386/addr-space-4.c b/gcc/testsuite/gcc.target/i386/addr-space-4.c
new file mode 100644
index 0000000..3e0966d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/addr-space-4.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+/* { dg-final { scan-assembler "gs:" } } */
+
+#define uintptr_t __SIZE_TYPE__
+
+struct S { int a, b, c; };
+
+extern struct S __seg_gs s;
+
+int foo (void)
+{
+  int r;
+  r = s.c;
+  return r;
+}
diff --git a/gcc/testsuite/gcc.target/i386/addr-space-5.c b/gcc/testsuite/gcc.target/i386/addr-space-5.c
new file mode 100644
index 0000000..4f73f95
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/addr-space-5.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+/* { dg-final { scan-assembler "gs:" } } */
+
+#define uintptr_t __SIZE_TYPE__
+
+struct S { int a, b, c; };
+
+extern struct S s;
+
+int ct_state3 (void)
+{
+  int r;
+  r = *((int __seg_gs *) (uintptr_t) &s.c);
+  return r;
+}
diff --git a/gcc/tree.c b/gcc/tree.c
index fa7646b..07cb9d9 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -12219,6 +12219,23 @@ block_ultimate_origin (const_tree block)
 bool
 tree_nop_conversion_p (const_tree outer_type, const_tree inner_type)
 {
+  /* 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;
+    }
+
   /* 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)

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

[Richard Biener]

On February 3, 2016 7:03:54 AM GMT+01:00, Richard Henderson <rth@redhat.com> wrote:
>In gimple_fold_indirect_ref, we STRIP_NOPS, find the ADDR_EXPR, and
>fold 
>everything away.
>
>I can't imagine it ever being correct to drop an address space change
>between 
>pointers, so I've modified tree_nop_conversion_p.  Anything else seems
>to 
>require more checks every places we use STRIP_NOPS.
>
>Ok?

I wasn't aware that STRIP_NOPS strips ADDR_SPACE_CONVERT_EXPR.

Isn't this maybe failing to use that (unable to look at the attachment from my phone).

Richard.

>
>r~

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

[Richard Henderson]

On 02/03/2016 06:05 PM, Richard Biener wrote:
  I wasn't aware that STRIP_NOPS strips ADDR_SPACE_CONVERT_EXPR.
>
> Isn't this maybe failing to use that (unable to look at the attachment from my phone).

The test case does fail to use ADDR_SPACE_CONVERT_EXPR.
Perhaps it's because of the intermediate cast to uintptr_t?

Of course, for this case, the intermediate cast is required
because __seg_[fg]s are *not* subsets of ADDR_SPACE_GENERIC,
and thus a direct cast between the pointer types results in
an error message.


r~

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

[Richard Biener]

On February 3, 2016 8:11:01 AM GMT+01:00, Richard Henderson <rth@redhat.com> wrote:
>On 02/03/2016 06:05 PM, Richard Biener wrote:
>  I wasn't aware that STRIP_NOPS strips ADDR_SPACE_CONVERT_EXPR.
>>
>> Isn't this maybe failing to use that (unable to look at the
>attachment from my phone).
>
>The test case does fail to use ADDR_SPACE_CONVERT_EXPR.
>Perhaps it's because of the intermediate cast to uintptr_t?

Ah.  Isn't to/from int conversion also address-space specific?

I wonder if it makes sense to have ADDR_SPACE_CONVERT if there is the loophole of going through an integer type...

That is, if the address spaces are not subsets, how can going through an int make sense? Isn't the testcase somehow invalid then?

>Of course, for this case, the intermediate cast is required
>because __seg_[fg]s are *not* subsets of ADDR_SPACE_GENERIC,
>and thus a direct cast between the pointer types results in
>an error message.

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.

Richard.

>
>r~

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

[Richard Henderson]

On 02/04/2016 12:46 AM, Richard Biener wrote:
> On February 3, 2016 8:11:01 AM GMT+01:00, Richard Henderson <rth@redhat.com> wrote:
>> On 02/03/2016 06:05 PM, Richard Biener wrote:
>>   I wasn't aware that STRIP_NOPS strips ADDR_SPACE_CONVERT_EXPR.
>>>
>>> Isn't this maybe failing to use that (unable to look at the
>> attachment from my phone).
>>
>> The test case does fail to use ADDR_SPACE_CONVERT_EXPR.
>> Perhaps it's because of the intermediate cast to uintptr_t?
>
> Ah.  Isn't to/from int conversion also address-space specific?

No, we just copy the bits there.

> I wonder if it makes sense to have ADDR_SPACE_CONVERT if there is the
> loophole of going through an integer type...

Dunno.

> That is, if the address spaces are not subsets, how can going through an int
> make sense? Isn't the testcase somehow invalid then?

Well, that depends.  For x86, they really are subsets, but the compiler does 
not know the relationship between them, so it cannot perform the conversion itself.

The test case is using implementation-defined conversions between pointers and 
integers (GCC defines the conversion to be bitwise).  So I don't think the test 
case is in any way invalid.

The user-friendly way to do this would probably be some sort of pragma that 
allows user-defined address spaces, and user-defined conversion between them. 
But that's certainly not going to happen in the near-term.

[ Irritatingly, there's a new Haswell instruction that would allow the compiler 
user-space access to the fs/gs_base registers, and then the compiler really 
would know the relationship.

However, the new instruction needs to be enabled by the OS, and not one has 
done so yet.  Then there's that further complication of all those non-Haswell 
cpus still running around.  So in practice we'd still want to be using the 
user-defined spaces. ]

> 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.


r~

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

[Richard Henderson]

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.



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

Re: [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
>

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

[Richard Henderson]

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~

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

[Richard Biener]

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~

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

[Richard Henderson]

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~

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

[Richard Biener]

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~

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

[Tom Tromey]

>>>>> "rth" == Richard Henderson <rth@redhat.com> writes:

rth> The user-friendly way to do this would probably be some sort of pragma
rth> that allows user-defined address spaces, and user-defined conversion
rth> between them. But that's certainly not going to happen in the
rth> near-term.

Related is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59850

When I was playing with this I think I tried initially to base the
user-defined address spaces on the built-in support.  I think I started
by trying to save part of the number space for user-defined spaces.
However, this got messy and in the end I just went with a separate
attribute... not very nice really.

Tom