Removing implicit phase reset for pointer-to-shared-array casting/assignment with differing referent type but matching ultimate element type

[Former user]

Originally reported on Google Code with ID 112

As proposed in Issue #3, when the address-of (&) operator is applied to an expression
of type pointer to shared array, the result is a pointer to the first element of the
array.

This seems straight forward, however consider the following:

<pre>
#include <upc.h>
#include <stdlib.h>
#include <stdio.h>


shared [5] int A[THREADS][6];
volatile int t = 1;
volatile int i = 4;

int
main ()
{
  shared [5] int *p1 = *&A[t];
  if (THREADS < 2)
    abort ();
  if (!MYTHREAD)
    {
      printf ("sizeof (*&A[t]) = %d\n", (int)sizeof(*&A[t]));
      printf ("sizeof (*p1) = %d\n", (int)sizeof(*p1));
      printf ("p1 = (0x%llx,t=%d,p=%d)\n", (long long) upc_addrfield (p1),
              (int) upc_threadof (p1), (int) upc_phaseof (p1));
    }
  return 0;
}
</pre>

When compiled and run under BUPC, this prints:

<pre>
sizeof (*&A[t]) = 24
sizeof (*p1) = 4
p1 = (0x3ff8014,t=1,p=1)
</pre>

When compiled with the current version of GUPC this prints:

<pre>
sizeof (*&A[t]) = 24
sizeof (*p1) = 4
p1 = (0x104,t=1,p=0)
</pre>

As shown, GUPC caused the phase to be reset upon assignment to p1.

The reason that GUPC chose this interpretation is illustrated by the following alternate
expression of the assignment to p1

 shared [5] int *p1 = (shared [5] int *)(shared [5] int (*)[6])&A[t];

The size of the array differs from the size of the element type.  In this case, the
UPC language specification states that the phase should be reset.

I don't know whether this is already covered by the update made to resolve issue #3.
 If not, perhaps some additional clarification is required?

GUPC has been modified such that the phase is not reset when converting from pointer
to shared araay to a pointer to the first element.

Reported by gary.funck on 2013-05-14 16:05:48

[Former user]

Here is the relevant text from UPC 6.4.3-2:
 The casting or assignment from one pointer-to-shared to another in which
 either the type size or block size differs, or either type is incomplete, results
 in a pointer with a zero phase...

> shared [5] int *p1 = *&A[t];

In the first declaration, the RHS of the declarator initializer expression has type
(shared [5] int [6]). This is an ARRAY type, not a pointer type. In fact by C99 6.5.3.2,
the combination "*&" basically has no effect whatsoever (aside from removing the l-valueness).
The initialization then implies an implicit conversion as specified in C99 6.3.2.1:

  an expression that has type ‘‘array of type’’ is
  converted to an expression with type ‘‘pointer to type’’ that points to the initial
element of
  the array object and is not an lvalue.

this implicit conversion results in type (shared [5] int *), which already exactly
matches the type of the variable p1. There is no mismatch in PTS blocksize or type
size anywhere here, so no phase reset should be performed. This seems like "right"
behavior, especially for the more common case of passing a sub-array to a function
argument declared as a PTS.

> The reason that GUPC chose this interpretation is illustrated by the following 
> alternate expression of the assignment to p1

 shared [5] int *p1 = (shared [5] int *)(shared [5] int (*)[6])&A[t];

This "alternate" assignment expression has a similar effect but is NOT semantically
identical. It is different because the result of &A[t] has pointer-to-shared type (shared
[5] int (*)[6]), which is then explicitly cast to a different pointer-to-shared type
(shared [5] int *), where the referenced type differs (an int instead of an array-of-int).
UPC requires a phase reset for that situation, and that also seems like the "right"
behavior, although it may be a bit surprising for this particular case.

So in summary I think this situation is already adequately covered, although it's a
bit subtle (as with most examples involving pointers-to-sub-arrays).

Reported by danbonachea on 2013-05-22 15:15:37

[Former user]

I think that since the phase works on the size of the ultimate element type for pointer-to-shared
arithmetic (see issue 3), pointers-to-shared where the type size differs but sizes
the ultimate element types do not probably should not reset the phase to zero.  I'd
therefore propose changing 6.4.3 2 from

"The casting or assignment from one pointer-to-shared to another in which either
the type size or block size differs..."

to

"The casting or assignment from one pointer-to-shared to another in which either
the size of the ultimate element type or block size differs..."

Reported by [e-mail] on 2013-05-22 17:44:28

[Former user]

"The casting or assignment from one pointer-to-shared to another in which either
the size of the ultimate element type or block size differs..."

Should probably also clarify this applies to the ultimate element type of the *referenced*
type and not the pointer itself, which is different (this was an ambiguity in the original
wording as well). Perhaps something like:

"The casting or assignment from one pointer-to-shared to another in which either
the block size or the size of the ultimate element type of the referenced type differs..."

It's worth noting that the sentence in question is already quite long, and might benefit
from some re-structuring for clarity?

Reported by danbonachea on 2013-05-22 17:58:30

[Former user]

This is one of the few unresolved technical issues still slated for the 1.3 milestone,
which has not been discussed in many months. We need to decide ASAP (hopefully by tomorrow's
call) whether to try and address this now or table it for 1.4.

We have a proposed resolution (comments #3/#4) that seems to make sense and is probably
the "best" long-term solution. My main concern is estimating the potential impact on
existing implementations and applications for tweaking the auto-phase-reset behavior.


I *think* the proposed change would only affect behavior when manipulating pointers
to array types (eg (shared [5] int (*)[6])), which probably already restricts the affected
clients considerably. Furthermore, the usage case that would see a behavioral change
is one where a cast/assign changes the referent's type size without changing the referent's
ultimate element type size, such as Gary's example:

 shared [5] int *p1 = (shared [5] int *)(shared [5] int (*)[6])&A[t];

Under the proposed change, this case would no longer imply an automatic phase reset,
where the 1.2 language implies it does (although the level of implementation compliance
to this corner case is questionable). I believe all other semantic situations are behaviorally
unchanged (right?). If an application contains code that falls into this affected case,
they can emulate the 1.2 behavior by inserting an explicit call to upc_resetphase(),
eg:

 shared [5] int *p1 = upc_resetphase(&A[t]);

This explicit form has the benefit of making the desired phase reset more obvious in
the code, which is also probably a Good Thing.

Would be nice to have input from some other implementers on this issue.

Reported by danbonachea on 2013-11-14 19:47:33

[Former user]

I'd very much like to see this fixed in 1.3, as we put a lot of effort into fixing issues
with multi-dimensional shared arrays and pointers to shared arrays. (As I side note,
I'll be unavailable to make the conference call due to an unexpected funeral).

> I *think* the proposed change would only affect behavior when manipulating pointers
to array types (eg (shared [5] int (*)[6])), which probably already restricts the affected
clients considerably.

I agree with this assessment.  I'd go further to say that the proposed change should
only affect codes that are already affected by issue 3 (and the issues that were spawned
from it), which are probably significantly more impacting.  As you point out, it is
trivial to work around this if needed.  More importantly, NOT fixing can lead to very
unintuitive situations.  Consider the following code based on Gary's example:

#include <upc.h>
#include <stdlib.h>
#include <stdio.h>


shared [5] int A[2*THREADS][6];
volatile int t = 1;
volatile int i = 4;

int
main ()
{
  shared [5] int *p1 = *&A[t];
  shared [5] int *p2 = &A[t][4];
  shared [5] int *p3 = (shared [5] int *)upc_resetphase(p1) + 4;
  shared [5] int *p4 = p1 + 4;
  if (THREADS < 2)
    abort ();
  if (!MYTHREAD)
    {
      printf ("p2 %2s p4\n",p2==p4?"==":"!=");
      printf ("p3 %2s p4\n",p3==p4?"==":"!=");
    }
  return 0;
}

In my mind, it seems more intuitive to require the output of this program to be

p2 == p4
p3 != p4

which ensures that incrementing p1 exactly follows the shared array that it was initialized
from.  This would then be consistent with the changes from issue 3.  I believe this
is what the proposed changes would require.

If instead we keep the reset, then creating a pointer that follows the shared array
becomes a lot more tricky, as you would need to manually rebuild the phase:

shared [5] int *p5 = ((shared [5] int*)upc_resetphase((*&A[t]) - (5*THREADS) + (5 -
upc_phaseof(&A[t])))) + upc_phaseof(&A[t]);

Notice that this is very tedious and error-prone given the lack of an ability to force
the phase to a value other than 0, which forces a pointer subtraction which could quite
easily fall prey to undefined behavior (underflow on the array bounds).

> Would be nice to have input from some other implementers on this issue.

Cray already follows the proposed behavior in CCE 8.2:

p2 == p4
p3 != p4

Reported by [e-mail] on 2013-11-14 20:53:35

[Former user]

> In my mind, it seems more intuitive to require the output of this program to be

Steve - I think we are in general agreement, regarding both the relevance of fixing
this in concert with the issue 3 changes, and also the correct and intuitive behavior.

However, I believe the test program you gave in comment #6 would NOT demonstrate the
proposed change in semantics, for the same reasons I outlined in comment #2 regarding
Gary's original example. Specifically, I believe your initialization expression for
p1:
   shared [5] int *p1 = *&A[t];
is required to preserve phase under both the new proposed semantics AND the original
semantics, because it falls under the special cases of C99's "*& erasure" and implicit
conversion for initialization of pointer expressions from array expressions. Stated
differently, this code never activates the "one pointer-to-shared to another" precondition
for both versions of the rule in question, so no implicit phase reset should be performed
in either case.

I would modify your test code as follows:

   shared [5] int (*p0)[6] = &A[t];
   shared [5] int *p1 = (shared [5] int *)p0;

The second line involves a cast from one PTS to another, where the size of the referent
type differs ('shared [5] int' vs 'shared [5] int[6]'), but the size of the ultimate
element type ('shared [5] int') does not. Thus under the 1.2 language, the second line
satisfies the rule precondition and requires a phase reset for the result in p1, whereas
under the proposed semantics it would not and phase should be preserved in p1.

Do you concur?

Reported by danbonachea on 2013-11-15 00:16:46

[Former user]

> Do you concur?

As you said, we are in agreement about the fix.  I'm not completely convinced of your
argument in comment #2 due to the wording "converted to an expression"--I thought that
conversion implies a cast.  I do agree that my program would not demonstrate the change
if I'm misunderstanding the difference between implicit conversions and implicit casts.
 In either case, your modification in comment #7 clearly illustrates the change without
that ambiguity.

Reported by [e-mail] on 2013-11-15 04:34:55

[Former user]

Below is a concrete latex-diff proposal for the wording discussed in comments 3/4. The
relevant technical change is:

+    \xreplaced[id=DB]{112}{
+    the block size or the size of the ultimate element type of the referenced type
differs,
+    }{ the type size or block size differs, }

In order to improve clarity, I have also broken the ridiculously long run-on sentence
into two separate sentences and swapped their position. This should have no semantic
impact, but I believe it greatly improves the readability of this paragraph. I've also
added a forward reference to the closely-related following paragraph (which could otherwise
be misinterpreted to contradict this one).

--- upc-language.tex    (revision 230)
+++ upc-language.tex    (working copy)
@@ -373,14 +373,18 @@

 {\bf Semantics}

-\np The casting or assignment from one pointer-to-shared 
-    to another in which either the type size or block size differs,
-    \xadded[id=DB]{70}{or either type is incomplete,}
-    results in 
-    a pointer with a zero phase, unless one of the types is a qualified or
-    unqualified version of {\tt shared void*}, the {\em generic pointer-to-shared},
-    in which case the phase is preserved unchanged in the resulting
-    pointer value.
+\np The casting or assignment from one pointer-to-shared to another 
+    where one of the types is a qualified or
+    unqualified version of {\tt shared void*}, the {\em generic pointer-to-shared},

+    preserves the phase component unchanged in the resulting pointer value
+    (except as discussed in the next paragraph).
+    The casting or assignment from one non-generic pointer-to-shared to another
+    in which either 
+    \xreplaced[id=DB]{112}{
+    the block size or the size of the ultimate element type of the referenced type
differs,
+    }{ the type size or block size differs, }
+    \xadded[id=DB]{70}{or either type is incomplete,} 
+    results in a pointer with a zero phase.

\np If a generic pointer-to-shared is cast to a non-generic
     pointer-to-shared type with indefinite block size or with block size of
     one, the result is a pointer with a phase of zero.  Otherwise, if the
     phase of the former pointer value is not within the range of possible
     phases of the latter pointer type, the result is undefined.

Reported by danbonachea on 2013-11-15 17:23:40

[Former user]

Proposal mailed 11/15/13

Reported by danbonachea on 2013-11-15 17:34:12 - Status changed: PendingApproval

[Former user]

On the 11/15 telecon, we resolved to forgo the 4-week comment period on this issue and
ratify it immediately, in the interests of timely ratification of the 1.3 spec.

Committed as SVN r233

Reported by danbonachea on 2013-11-15 19:13:48 - Status changed: Ratified