X10

VectorTest: expression flattener interferes with mutable structs

Details

  • Number of attachments :
    0

Description

The expression flattener is interfering with the Vec class. This is particularly problematic in the java backend where the flattener is on by default.

The flattener causes an extra value assignment in the case of vec_sized1(15) = a; that causes the side-effect to be lost

//#line 28 "/home/spark/work/x10-dbg/x10.tests/examples/Constructs/Structs/VectorTest.x10"
final x10.core.Vec<$T> t2438 =
              ((x10.core.Vec)(vec_sized1)).clone();

//#line 28 "/home/spark/work/x10-dbg/x10.tests/examples/Constructs/Structs/VectorTest.x10"
x10.runtime.util.Util.eval(((x10.core.Vec<$T>)(t2438)).set(((int)(15)),a));

also

    //#line 28 "/home/spark/work/x10-dbg/x10.tests/examples/Constructs/Structs/VectorTest.x10": x10.ast.X10LocalDecl_c
    x10::util::NativeVec<TPMGL(T), 20> t2438 = vec_sized1;

    //#line 28 "/home/spark/work/x10-dbg/x10.tests/examples/Constructs/Structs/VectorTest.x10": polyglot.ast.Eval_c
    t2438.set(a,((x10_int)15));

I am not sure how to fix this.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Dave Cunningham added a comment -

The flattener could avoid flattening lvalues.

A simple definition of lvalue that would suffice for VectorTest would be 'an lvalue is a variable or parameter'

I don't see there is any reason to flatten such things anyway, so this would help with readability.

Show
Dave Cunningham added a comment - The flattener could avoid flattening lvalues. A simple definition of lvalue that would suffice for VectorTest would be 'an lvalue is a variable or parameter' I don't see there is any reason to flatten such things anyway, so this would help with readability.
Hide
Permalink
Dave Cunningham added a comment -

The following patch seems to fix it but I want to verify that it has no unintended side-effects.

Index: x10.compiler/src/x10/visit/ExpressionFlattener.java
===================================================================
--- x10.compiler/src/x10/visit/ExpressionFlattener.java (revision 22025)
+++ x10.compiler/src/x10/visit/ExpressionFlattener.java (working copy)
@@ -1372,7 +1372,7 @@
     private boolean isPrimary(Expr expr) {
         if (null == expr) return true; // DEBUG
         if (expr instanceof Lit) return true;
-        if (expr instanceof Local) return ((Local) expr).flags().contains(Flags.FINAL);
+        if (expr instanceof Local) return true;
         if (expr instanceof Special) return true;
         if (expr instanceof Here) return true;
         return false;
Show
Dave Cunningham added a comment - The following patch seems to fix it but I want to verify that it has no unintended side-effects. Index: x10.compiler/src/x10/visit/ExpressionFlattener.java =================================================================== --- x10.compiler/src/x10/visit/ExpressionFlattener.java (revision 22025) +++ x10.compiler/src/x10/visit/ExpressionFlattener.java (working copy) @@ -1372,7 +1372,7 @@ private boolean isPrimary(Expr expr) { if ( null == expr) return true ; // DEBUG if (expr instanceof Lit) return true ; - if (expr instanceof Local) return ((Local) expr).flags().contains(Flags.FINAL); + if (expr instanceof Local) return true ; if (expr instanceof Special) return true ; if (expr instanceof Here) return true ; return false ;
Hide
Permalink
Dave Cunningham added a comment -

Summary of discussion with Bowen & DG:

Some structs have @Mutable annotation, meaning x.f = 10 is allowed (note: front end rejects this at the moment).

Some methods of such structs have @Mutable annotation, meaning this.f = 10 is allowed (note: front end rejects this, but @Native annotation can do the update, which means for optimisation purposes we have to assume that @Mutable methods can update the struct's fields).

@Mutable methods need the receiver to be passed by reference. All parameters are still passed by value.

@Mutable methods may only be called on a receiver expression that is an lvalue.

At this time, an expression is an lvalue iff it is a local or parameter.

When flattening a method call, if the receiver is by reference, it should not be hoisted from the call expression, i.e. the transformation should be as follows:

et.m(e1,e2,e3)

=>

IF (m @Mutable) THEN
val t1 = e1;
val t2 = e2;
val t3 = e3;
et.m(t1,t2,t3);
ELSE
val tt = et;
val t1 = e1;
val t2 = e2;
val t3 = e3;
tt.m(t1,t2,t3);
ENDIF

Note: even if e1,e2,e3 refer to x, e.g. the example

x.m(x=y)

is O.K. to be transformed as per the @Mutable rule above, even though the x=y assignment is moved in front of the target expression which evaluates x. This is because the target is byref, the semantics is that the target is not actually evaluated, but a pointer to it is used. For illustration, this is the equivalent C code:

m(&x, x=y)

Clearly, x=y will not change the value of the expression &e.

Show
Dave Cunningham added a comment - Summary of discussion with Bowen & DG: Some structs have @Mutable annotation, meaning x.f = 10 is allowed (note: front end rejects this at the moment). Some methods of such structs have @Mutable annotation, meaning this.f = 10 is allowed (note: front end rejects this, but @Native annotation can do the update, which means for optimisation purposes we have to assume that @Mutable methods can update the struct's fields). @Mutable methods need the receiver to be passed by reference. All parameters are still passed by value. @Mutable methods may only be called on a receiver expression that is an lvalue. At this time, an expression is an lvalue iff it is a local or parameter. When flattening a method call, if the receiver is by reference, it should not be hoisted from the call expression, i.e. the transformation should be as follows: et.m(e1,e2,e3) => IF (m @Mutable) THEN val t1 = e1; val t2 = e2; val t3 = e3; et.m(t1,t2,t3); ELSE val tt = et; val t1 = e1; val t2 = e2; val t3 = e3; tt.m(t1,t2,t3); ENDIF Note: even if e1,e2,e3 refer to x, e.g. the example x.m(x=y) is O.K. to be transformed as per the @Mutable rule above, even though the x=y assignment is moved in front of the target expression which evaluates x. This is because the target is byref, the semantics is that the target is not actually evaluated, but a pointer to it is used. For illustration, this is the equivalent C code: m(&x, x=y) Clearly, x=y will not change the value of the expression &e.
Hide
Permalink
Mikio Takeuchi added a comment -

VectorTest is still failing with x10c (without -O).
Who is maintaining ExpressionFlattener after Bowen's leave from IBM?

Show
Mikio Takeuchi added a comment - VectorTest is still failing with x10c (without -O). Who is maintaining ExpressionFlattener after Bowen's leave from IBM?
Hide
Permalink
David Grove added a comment -

bulk defer of open issues to 2.2.2.

Show
David Grove added a comment - bulk defer of open issues to 2.2.2.
Hide
Permalink
David Grove added a comment -

bulk defer of issues to 2.2.3.

Show
David Grove added a comment - bulk defer of issues to 2.2.3.
Hide
Permalink
Mikio Takeuchi added a comment -

Currently managed x10 calls clone() at every copy of mutable structs. Not clear what is the right thing to do.

Show
Mikio Takeuchi added a comment - Currently managed x10 calls clone() at every copy of mutable structs. Not clear what is the right thing to do.
Hide
Permalink
David Grove added a comment -

defer to 2.3.1; some chance that in the 2.3.1 timeframe we'll make Vec less needed by reworking IndexedMemoryChunk into a "built-in" dense, zero-based multi-dimensional array.

Show
David Grove added a comment - defer to 2.3.1; some chance that in the 2.3.1 timeframe we'll make Vec less needed by reworking IndexedMemoryChunk into a "built-in" dense, zero-based multi-dimensional array.
Hide
Permalink
David Grove added a comment -

bulk defer to 2.3.2

Show
David Grove added a comment - bulk defer to 2.3.2

People

  • Assignee:
    David Grove
    Reporter:
    Dave Cunningham
Vote (0)
Watch (2)

Dates

  • Created:
    Updated: