signed multiplication for pointer offsets

signed multiplication for pointer offsets

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1451 bytes --]

Hello,

when we have a double*p, computing p+n, we multiply n by 8 (size of 
double) then add it to p. According to the comment just below the modified 
lines in the attached patch, the multiplication is supposed to happen in a 
signed type. However, that does not match the current code which uses 
sizetype. This is causing missed optimizations, for instance, when 
comparing p+m and p+n, we might end up comparing 8*m to 8*n, which is not 
the same as comparing m and n if overflow can happen.

I tried this patch to see how much breaks. And actually, not that much 
does. There are a few fragile testcases that want to match "q_. " but it 
is now "q_10 ", or they expect 3 simplifications and get 5 (same final 
code). One was confused by a cast in the middle of x+cst1+cst2. A couple 
were hit by the lack of CSE between (x+1)*8, x*8+8, and some variants with 
casts in the middle, but that's already an issue without the patch. A few 
vectorization testcases failed because SCEV did not recognize a simple 
increment of a variable with NOP casts in the middle, that would be worth 
fixing. The patch actually fixed another vectorization testcase.

I guess it would help if pointer_plus switched to a signed second 
argument, to be consistent with this and reduce the number of casts.

I am not proposing the patch as is, but is this the direction we want to 
follow, or should I just fix the comment to match what the code does?

-- 
Marc Glisse

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name=ssize.patch, Size: 1816 bytes --]

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 248308)
+++ gcc/c-family/c-common.c	(working copy)
@@ -3106,24 +3106,24 @@ pointer_int_sum (location_t loc, enum tr
 	 because weird cases involving pointer arithmetic
 	 can result in a sum or difference with different type args.  */
       ptrop = build_binary_op (EXPR_LOCATION (TREE_OPERAND (intop, 1)),
 			       subcode, ptrop,
 			       convert (int_type, TREE_OPERAND (intop, 1)), 1);
       intop = convert (int_type, TREE_OPERAND (intop, 0));
     }
 
   /* Convert the integer argument to a type the same size as sizetype
      so the multiply won't overflow spuriously.  */
-  if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (sizetype)
-      || TYPE_UNSIGNED (TREE_TYPE (intop)) != TYPE_UNSIGNED (sizetype))
-    intop = convert (c_common_type_for_size (TYPE_PRECISION (sizetype),
-					     TYPE_UNSIGNED (sizetype)), intop);
+  if (TYPE_PRECISION (TREE_TYPE (intop)) != TYPE_PRECISION (ssizetype)
+      || TYPE_UNSIGNED (TREE_TYPE (intop)) != TYPE_UNSIGNED (ssizetype))
+    intop = convert (c_common_type_for_size (TYPE_PRECISION (ssizetype),
+					     TYPE_UNSIGNED (ssizetype)), intop);
 
   /* Replace the integer argument with a suitable product by the object size.
      Do this multiplication as signed, then convert to the appropriate type
      for the pointer operation and disregard an overflow that occurred only
      because of the sign-extension change in the latter conversion.  */
   {
     tree t = fold_build2_loc (loc, MULT_EXPR, TREE_TYPE (intop), intop,
 			      convert (TREE_TYPE (intop), size_exp));
     intop = convert (sizetype, t);
     if (TREE_OVERFLOW_P (intop) && !TREE_OVERFLOW (t))

Re: signed multiplication for pointer offsets

[Richard Biener]

On Sun, May 21, 2017 at 9:45 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> when we have a double*p, computing p+n, we multiply n by 8 (size of double)
> then add it to p. According to the comment just below the modified lines in
> the attached patch, the multiplication is supposed to happen in a signed
> type. However, that does not match the current code which uses sizetype.
> This is causing missed optimizations, for instance, when comparing p+m and
> p+n, we might end up comparing 8*m to 8*n, which is not the same as
> comparing m and n if overflow can happen.
>
> I tried this patch to see how much breaks. And actually, not that much does.
> There are a few fragile testcases that want to match "q_. " but it is now
> "q_10 ", or they expect 3 simplifications and get 5 (same final code). One
> was confused by a cast in the middle of x+cst1+cst2. A couple were hit by
> the lack of CSE between (x+1)*8, x*8+8, and some variants with casts in the
> middle, but that's already an issue without the patch. A few vectorization
> testcases failed because SCEV did not recognize a simple increment of a
> variable with NOP casts in the middle, that would be worth fixing. The patch
> actually fixed another vectorization testcase.
>
> I guess it would help if pointer_plus switched to a signed second argument,
> to be consistent with this and reduce the number of casts.

Yes.  In fact the pointer-plus offset is to be interpreted as signed anyway
so that it uses unsigned sizetype was a bad decision back in time.  Bin
tried to fix this last year but I'm not sure what came of his experiments.

> I am not proposing the patch as is, but is this the direction we want to
> follow, or should I just fix the comment to match what the code does?

I think I tried this at some point and I don't remember the result.  I did that
in the context of the even more ambitious attempt to make pointer-plus
take either sizetype or ssizetype typed offsets...

The standard doesn't actually talk about what type to perform the multiplication
in but given we effectively restrict objects to the size of half of
the address space
due to ptrdiff_t limitations using either a signed or unsigned type should work.

Richard.

> --
> Marc Glisse