Support conditional copy-by-reference in local memory #462

mairooni · 2024-06-25T13:44:58Z

Description

This PR adds support for conditional copy-by-reference in local memory.

Problem description

If a plain copy-by-reference occurs in local memory, graal updates the references automatically. For example, the code below is legal and functionally correct:

public static void intPrivateCopyNoCondition(IntArray a, IntArray b) {
        for (@Parallel int i = 0; i < a.getSize(); i++) {
            int[] arrayA = new int[128];
            int[] arrayB = new int[128];
            for (int j = 0; j < 128; j++) {
                arrayA[j] = j;
                arrayB[j] = 2;
            }

            // copy arrayA to arrayB
            arrayB = arrayA;

            b.set(i, arrayB[i]);
        }
}

However, if the copy takes place depending on a condition, e.g.:

 if ((a.get(i) % 2) == 0 ) {
       // if condition is true, copy arrayA to arrayB
        arrayB = arrayA;
 }

Tornado produces a compiler error.
This PR adds the necessary functionality to make this conditional copy-by-reference legal. This patch is for OpenCL and PTX, SPIR-V needs further investigation.

Backend/s tested

Mark the backends affected by this PR.

OpenCL
PTX
SPIRV

OS tested

Mark the OS where this PR is tested.

Linux
OSx
Windows

Did you check on FPGAs?

If it is applicable, check your changes on FPGAs.

Yes
No

How to test the new patch?

Run
tornado-test -V --fast uk.ac.manchester.tornado.unittests.arrays.TestArrayCopies

…ed for local array conditional copy

… it is not currently supported

…ry, as this is not supported

…tion

jjfumero · 2024-06-26T08:05:52Z

[Review in progress] Test are passing for OpenCL and PTX.

jjfumero · 2024-06-26T08:22:43Z

...in/java/uk/ac/manchester/tornado/drivers/opencl/graal/phases/TornadoFixedArrayCopyPhase.java

+
+    protected void run(StructuredGraph graph) {
+        for (ValuePhiNode phiNode : graph.getNodes().filter(ValuePhiNode.class)) {
+            if (phiNode.usages().filter(OffsetAddressNode.class).isNotEmpty() && phiNode.values().filter(FixedArrayNode.class).isNotEmpty()) {


Add a method that returns true or false using these condition.

jjfumero · 2024-06-26T08:24:11Z

...in/java/uk/ac/manchester/tornado/drivers/opencl/graal/phases/TornadoFixedArrayCopyPhase.java

+                // finally, since we know that the data accessed is a fixed array, fix the offset
+                ValuePhiNode privateIndex = getPrivateArrayIndex(offsetAddressNode.getOffset());
+                offsetAddressNode.setOffset(privateIndex);
+                break;


Remove the break, we want to explore all copies within the kernel.

jjfumero · 2024-06-26T08:26:13Z

...in/java/uk/ac/manchester/tornado/drivers/opencl/graal/phases/TornadoFixedArrayCopyPhase.java

+                offsetAddressNode.replaceFirstInput(phiNode, fixedArrayCopyNode);
+                // finally, since we know that the data accessed is a fixed array, fix the offset
+                ValuePhiNode privateIndex = getPrivateArrayIndex(offsetAddressNode.getOffset());
+                offsetAddressNode.setOffset(privateIndex);


Can privateIndex be null?

jjfumero · 2024-06-26T08:30:00Z

...s/ptx/src/main/java/uk/ac/manchester/tornado/drivers/ptx/graal/nodes/FixedArrayCopyNode.java

+            result = tool.newVariable(lirKind);
+            tool.append(new PTXLIRStmt.AssignStmt(result, new PTXUnary.Expr(PTXAssembler.PTXUnaryOp.CVT_FLOAT, lirKind, baseAddress)));
+            declarationPtr = new PTXBinary.Expr(pointerCopyTemplate, lirKind, result, baseAddress);
+    } else {


Check the formatter

jjfumero · 2024-06-26T08:30:45Z

.../main/java/uk/ac/manchester/tornado/drivers/ptx/graal/phases/TornadoFixedArrayCopyPhase.java

+                    offsetAddressNode.safeDelete();
+                }
+                phiNodeToReplace = phiNode;
+                break;


remove the break to explore all copies.

jjfumero · 2024-06-26T08:33:23Z

.../main/java/uk/ac/manchester/tornado/drivers/ptx/graal/phases/TornadoFixedArrayCopyPhase.java

+                    deleteFixed(readNode);
+                    offsetAddressNode.safeDelete();
+                }
+                phiNodeToReplace = phiNode;


Replace this with a LinkedList of phoNodes to be replaced after the for loop

jjfumero · 2024-06-26T08:34:46Z

.../main/java/uk/ac/manchester/tornado/drivers/ptx/graal/phases/TornadoFixedArrayCopyPhase.java

+                PTXArchitecture.PTXMemoryBase oclMemoryBase = fixedArrayNode.getMemoryRegister();
+                OffsetAddressNode offsetAddressNode = phiNode.usages().filter(OffsetAddressNode.class).first();
+                ValuePhiNode privateIndex = getPrivateArrayIndex(offsetAddressNode.getOffset());
+                FixedArrayCopyNode fixedArrayCopyNode = new FixedArrayCopyNode(phiNode, resolvedJavaType, oclMemoryBase, privateIndex);


Check privateIndex is not null.

jjfumero · 2024-06-26T08:34:55Z

tornado-unittests/src/main/java/uk/ac/manchester/tornado/unittests/arrays/TestArrayCopies.java

@@ -0,0 +1,276 @@
+package uk.ac.manchester.tornado.unittests.arrays;


Add license

jjfumero · 2024-06-26T08:38:17Z

Throw exception when this type of code is executed for the SPIR-V backend. This can be done within the a compilation phase.

mairooni · 2024-06-26T09:26:40Z

The comments have been addressed. @jjfumero please have another look.

jjfumero · 2024-06-26T09:29:21Z

Great. Thanks Mary!

mikepapadim

LGTM, thanks @mairooni

Some minor refactoring to improve code with pattern matching if you have time

mikepapadim · 2024-06-26T10:50:32Z

...-drivers/opencl/src/main/java/uk/ac/manchester/tornado/drivers/opencl/graal/lir/OCLKind.java

+    public static OCLAssembler.OCLBinaryTemplate resolvePrivatePointerTemplate(JavaKind type) {
+        if (type == JavaKind.Int) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_INT_ARRAY_PTR;
+        } else if (type == JavaKind.Double) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_DOUBLE_ARRAY_PTR;
+        } else if (type == JavaKind.Float) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_FLOAT_ARRAY_PTR;
+        } else if (type == JavaKind.Short) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_SHORT_ARRAY_PTR;
+        } else if (type == JavaKind.Long) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_LONG_ARRAY_PTR;
+        } else if (type == JavaKind.Char) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_CHAR_ARRAY_PTR;
+        } else if (type == JavaKind.Byte) {
+            return OCLAssembler.OCLBinaryTemplate.PRIVATE_BYTE_ARRAY_PTR;
+        }
+        return null;
+    }


Suggested change

public static OCLAssembler.OCLBinaryTemplate resolvePrivatePointerTemplate(JavaKind type) {

if (type == JavaKind.Int) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_INT_ARRAY_PTR;

} else if (type == JavaKind.Double) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_DOUBLE_ARRAY_PTR;

} else if (type == JavaKind.Float) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_FLOAT_ARRAY_PTR;

} else if (type == JavaKind.Short) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_SHORT_ARRAY_PTR;

} else if (type == JavaKind.Long) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_LONG_ARRAY_PTR;

} else if (type == JavaKind.Char) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_CHAR_ARRAY_PTR;

} else if (type == JavaKind.Byte) {

return OCLAssembler.OCLBinaryTemplate.PRIVATE_BYTE_ARRAY_PTR;

}

return null;

}

public static OCLAssembler.OCLBinaryTemplate resolvePrivatePointerTemplate(JavaKind type) {

return switch (type) {

case Int -> OCLAssembler.OCLBinaryTemplate.PRIVATE_INT_ARRAY_PTR;

case Double -> OCLAssembler.OCLBinaryTemplate.PRIVATE_DOUBLE_ARRAY_PTR;

case Float -> OCLAssembler.OCLBinaryTemplate.PRIVATE_FLOAT_ARRAY_PTR;

case Short -> OCLAssembler.OCLBinaryTemplate.PRIVATE_SHORT_ARRAY_PTR;

case Long -> OCLAssembler.OCLBinaryTemplate.PRIVATE_LONG_ARRAY_PTR;

case Char -> OCLAssembler.OCLBinaryTemplate.PRIVATE_CHAR_ARRAY_PTR;

case Byte -> OCLAssembler.OCLBinaryTemplate.PRIVATE_BYTE_ARRAY_PTR;

default -> null;

};

}

I think can be done with pattern matching for simplicity

not sure if this suggestion is correct.

mikepapadim · 2024-06-26T10:51:13Z

...-drivers/opencl/src/main/java/uk/ac/manchester/tornado/drivers/opencl/graal/lir/OCLKind.java

+        return null;
+    }
+
+    public static OCLAssembler.OCLBinaryTemplate resolvePrivatePointerCopyTemplate(JavaKind type) {


mikepapadim · 2024-06-26T10:52:17Z

tornado-drivers/ptx/src/main/java/uk/ac/manchester/tornado/drivers/ptx/graal/lir/PTXKind.java

+        return resolvePrivatePointerCopyTemplate(type.getJavaKind());
+    }
+
+    public static PTXAssembler.PTXBinaryTemplate resolvePrivatePointerCopyTemplate(JavaKind type) {


here maybe also you can use pattern matching

mikepapadim · 2024-06-26T10:53:12Z

...s/ptx/src/main/java/uk/ac/manchester/tornado/drivers/ptx/graal/nodes/FixedArrayCopyNode.java

+        return memoryRegister;
+    }
+
+    private int getOffset() {


Suggested change

private int getOffset() {

private int getOffset() {

PTXKind kind = PTXKind.fromResolvedJavaType(elementType);

return switch (kind) {

case F64, S64 -> 8;

case F32, S32 -> 4;

case S16 -> 2;

case U16, S8 -> 1;

default -> -1;

};

}

mairooni · 2024-06-26T11:23:23Z

The refactoring has been applied, thanks @mikepapadim!

jjfumero · 2024-06-26T11:24:23Z

Great, so we merge this.

mairooni added 18 commits May 16, 2024 13:35

Generate pointer that points to private array

606fc08

[OpenCL] Enable private array copies by reference

b0d5022

Merge branch 'develop' into feat/pointer_copies

aca4493

Initial support for pointer copy in private memory for PTX

f397240

[WIP] Fix bug in codegen for PTX reference copy

d4b29d0

[WIP] Copy by reference in private memory for SPIRV

b5fb599

Merge branch 'develop' into feat/pointer_copies

7bdd801

Fix bug in PTX for fixed array reference copy

2c753d8

Refactoring in the PTX backend to ensure that mov operator is generat…

16161fe

…ed for local array conditional copy

Merge branch 'develop' into feat/pointer_copies

c6eef9c

Refactor unittest to use the API try-with-resources

e1bb5b9

Throw an exception when unittest is executed on the SPIRV backend, as…

3aaaf21

… it is not currently supported

Fix checktyle violation

9e03609

Undo changes in the SPIRV backend for copy-by-refence in private memo…

72500a9

…ry, as this is not supported

Fix checkstyle

fe0dd2e

Remove unnecessary conditions

42df891

Minor refactoring of variable names

a29f1ca

Fix Comment

675db13

mairooni added compiler OpenCL PTX fix Provides a fix labels Jun 25, 2024

mairooni requested review from jjfumero and mikepapadim June 25, 2024 13:44

mairooni self-assigned this Jun 25, 2024

Include a unittest that performs copy in local memory without a condi…

62443f4

…tion

jjfumero added feature New feature proposal and removed fix Provides a fix labels Jun 26, 2024

jjfumero requested changes Jun 26, 2024

View reviewed changes

mairooni added 2 commits June 26, 2024 12:16

Apply code review comments

6e381c2

Include missing import and add phase in the low tier

dd45e60

jjfumero approved these changes Jun 26, 2024

View reviewed changes

Merge branch 'develop' into feat/pointer_copies

f84aa53

mikepapadim approved these changes Jun 26, 2024

View reviewed changes

Minor refactoring to use pattern matching

fadb5ea

jjfumero merged commit 1be5451 into beehive-lab:develop Jun 26, 2024
2 checks passed

mairooni deleted the feat/pointer_copies branch June 26, 2024 11:25

jjfumero mentioned this pull request Jun 27, 2024

[release] TornadoVM v1.0.6 #464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support conditional copy-by-reference in local memory #462

Support conditional copy-by-reference in local memory #462

mairooni commented Jun 25, 2024 •

edited by jjfumero

Loading

jjfumero commented Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero Jun 26, 2024

jjfumero commented Jun 26, 2024

mairooni commented Jun 26, 2024

jjfumero commented Jun 26, 2024

mikepapadim left a comment

mikepapadim Jun 26, 2024

mikepapadim Jun 26, 2024

mikepapadim Jun 26, 2024

mikepapadim Jun 26, 2024

mikepapadim Jun 26, 2024

mikepapadim Jun 26, 2024

mairooni commented Jun 26, 2024

jjfumero commented Jun 26, 2024

		@@ -0,0 +1,276 @@
		package uk.ac.manchester.tornado.unittests.arrays;

-    private int getOffset() {
+    private int getOffset() {
+    PTXKind kind = PTXKind.fromResolvedJavaType(elementType);
+    return switch (kind) {
+        case F64, S64 -> 8;
+        case F32, S32 -> 4;
+        case S16 -> 2;
+        case U16, S8 -> 1;
+        default -> -1;
+    };
+}

Support conditional copy-by-reference in local memory #462

Support conditional copy-by-reference in local memory #462

Conversation

mairooni commented Jun 25, 2024 • edited by jjfumero Loading

Description

Problem description

Backend/s tested

OS tested

Did you check on FPGAs?

How to test the new patch?

jjfumero commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjfumero commented Jun 26, 2024

mairooni commented Jun 26, 2024

jjfumero commented Jun 26, 2024

mikepapadim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mairooni commented Jun 26, 2024

jjfumero commented Jun 26, 2024

mairooni commented Jun 25, 2024 •

edited by jjfumero

Loading