ARRAY_REF patch

[Mark Mitchell <mark at codesourcery dot com>]

Danny --

[This time posted to the correct address.]

You posted this patch:

http://gcc.gnu.org/ml/gcc-patches/2005-06/msg02025.html

before the close of Stage 2.  The key idea behind this patch is to 
preserve the distinction between "*(p + offset)" and "a[i]" for the 
optimizers, which everyone agrees is a laudable goal.

Since you didn't get much of a reaction, I decided to review the patch.

Do we presently have optimizations which can presently take advantage of 
the distinction?  In other words, does your patch result in better code? 
 If it does not, then I don't immediately see any reason to include it 
in 4.1; it seems like it should just go into 4.2 as infrastructure. 
However, if it does generate better code then, as it was submitted 
before the end of Stage 2, I think we should consider it for 4.1.

On the technical side, my most basic question is about the 
representation you chose.  Why did you choose to have the first operand 
be a POINTER_TYPE rather than an ARRAY_TYPE?  I think a better tree 
representation would be to use the tree equivalent of:

  *((T (*)[]) p)

as the first operand to the ARRAY_REF.  That would give the first 
operand an ARRAY_TYPE, as expected everywhere else in the compiler at 
present.  It would make me much less nervous that we're going to have 
places that are failing to check POINTER_ARRAY_ACCESS_P.  I would also 
be OK with creating a new node (POINTER_ARRAY_REF?) to handle this 
usage, if you think that's more convenient; what worries me is that 
you're weakening the present type system.  If we do use your 
representation, then you're missing changes to tree.def that describe 
the variant form of ARRAY_REF.

Also:

+      /* If this type has no size, either we're screwed or we've issued an
+     error, so building an ARRAY_REF or not isn't going to help
+     us.  */

First, please avoid using language some find offensive.  Second, please 
say what you really mean.  Why are we screwed?  Do you really mean "if 
the size of this type cannot be determined until run-time, the 
optimizers will not be able to take advantage of the fact that this is 
an array access"?  Or...?

Also:

-      if (!c_mark_addressable (TREE_OPERAND (arg, 0)))
+      /* The pointer array form is equivalent to *(a + offset),
+         so &(*(a + something)) is  just a + offset, not an
+         addressable op.  */
+      if (!POINTER_ARRAY_ACCESS_P (arg)
+          && !c_mark_addressable (TREE_OPERAND (arg, 0)))
         return error_mark_node;
       return build_binary_op (PLUS_EXPR,
                   default_function_array_conversion
                     (TREE_OPERAND (arg, 0)),
                   TREE_OPERAND (arg, 1), 1);
     }
-
+
       /* Anything not already handled and not a true memory reference
      or a non-lvalue array is an error.  */
       else if (typecode != FUNCTION_TYPE && !flag
@@ -2886,8 +2910,10 @@ c_mark_addressable (tree exp)

     /* ... fall through ...  */

-      case ADDR_EXPR:
       case ARRAY_REF:
+    if (POINTER_ARRAY_ACCESS_P (x))
+      return true;

This looks redundant.  If you've changed c_mark_addressable to handle 
ARRAY_REFs that are POINTER_ARRAY_ACCESS_P, then why do you need to 
guard the call to c_mark_addressable?

--
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304