Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OpenCL] Fix issues with brackets for if-conditions in for-loops #571

Merged
merged 5 commits into from
Oct 4, 2024

Conversation

mairooni
Copy link
Collaborator

@mairooni mairooni commented Oct 1, 2024

Description

This PR fixes the closing brackets for OpenCL kernels that contain if-conditions either before or in for-loops.

Problem description

It was observed that in some cases where there was an if-condition in a kernel that contained for-loop(s) (e.g. before a for-loop, in an inner for-loop etc.) , either some closing brackets were missing or unnecessary additional closing brackets were included. With this PR, the code generation examines these cases and generates the correct brackets.

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 -pk uk.ac.manchester.tornado.unittests.branching.TestLoopConditions
All unittests have been tested along with RayTracer and KFusion.

@mairooni mairooni added bug Something isn't working OpenCL labels Oct 1, 2024
@mairooni mairooni self-assigned this Oct 1, 2024
@stratika
Copy link
Collaborator

stratika commented Oct 1, 2024

Just a note, I tested the branch in macOSx and Linux Ubuntu 23.10 and the fix is working.

@mairooni mairooni requested a review from stratika October 1, 2024 12:53
@mairooni
Copy link
Collaborator Author

mairooni commented Oct 1, 2024

Just a note, I tested the branch in macOSx and Linux Ubuntu 23.10 and the fix is working.

Thanks for testing it Thano!

@jjfumero
Copy link
Member

jjfumero commented Oct 1, 2024

That's great. Based on my experience when we fix the OpenCL visitor is that we always need to check with KFusion.

@stratika
Copy link
Collaborator

stratika commented Oct 1, 2024

That's great. Based on my experience when we fix the OpenCL visitor is that we always need to check with KFusion.

Good note! Just tested kfusion with OpenCL, PTX on NVIDIA RTX A2000 GPU.

I also tested TornadoVM-Ray-Tracer for OpenCL.

Comment on lines 46 to 48
if (a.get(0) > b.get(0)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected behaviour of this type of code when running in parallel on a GPU? We can't stop the GPU kernel.

Comment on lines +65 to +67
if (a.get(0) > b.get(0)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, what is the expected behaviour when running on a GPU?

Copy link
Member

@jjfumero jjfumero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the sync, let's clarify in the header of the test and before each kernel why this test is valid and under which conditions will be still valid for TornadoVM.

@jjfumero jjfumero merged commit 637f358 into beehive-lab:develop Oct 4, 2024
2 checks passed
jjfumero added a commit to jjfumero/TornadoVM that referenced this pull request Dec 20, 2024
Improvements
=============

- beehive-lab#573: Enhanced output of unit-tests with a summary  of pass-rates and fail-rates.
- beehive-lab#576: Extended support for 3D matrices.
- beehive-lab#580: Extended debug information for execution plans.
- beehive-lab#584: Added helper menu for the ``tornado`` launcher script when no arguments are passed.
- beehive-lab#589: Enable partial loop unrolling for all backends.
- beehive-lab#594: Added RISC-V 64 CPU port support to run OpenCL with vector instructions RVV 1.0 (using the Codeplay OCK Toolkit).
- beehive-lab#598: OpenCL low-level buffers tagged as read, write and read/write based on the data dependency analysis.
- beehive-lab#601: Feature to select an immutable task graph to execute from a multi-task graph execution plan.

Compatibility
=============

- beehive-lab#570:  Extended timeout for all suite of unit-tests.
- beehive-lab#579: Removed legacy JDK 8 and JDK11 build options from the TornadoVM installer.
- beehive-lab#582: Restored tornado runner scripts for IntellIJ.
- beehive-lab#583: Automatic generation of IDE IntelliJ configuration runner files from the TornadoVM command.
- beehive-lab#597: Updated white-list of unit-test and checkstyle improved.

Bug Fixes
=============

- beehive-lab#571: Fix issues with bracket closing for if/loops conditions.
- beehive-lab#572: Fix for printing default execution plans (execution plans with default parameters).
- beehive-lab#575: Fix the Level Zero version used for building the SPIR-V backend.
- beehive-lab#577: Fix checkstyle.
- beehive-lab#587: Fix thread scheduler for new NVIDIA Drivers.
- beehive-lab#592: Fix ``Float.POSITIVE_INFINITY`` and ``Float.NEGATIVE_INFINITIVE`` constants for the OpenCL, CUDA and SPIR-V backends.
- beehive-lab#596: Fix extra closing bracket during the code-generation for the FPGAs.
- Remove the intermediate CUDA pinned memory regions in the JNI code: [link](beehive-lab@9c3f8ce)
- Fix bitwise negation operations for the PTX backend:  [link](beehive-lab@0db1cd3)
- `GetBackendImpl::getAllDevices` thread-safe: [link](beehive-lab@0d44252)
- Check size elements for memory segments: [link](beehive-lab@4360385)
@jjfumero jjfumero mentioned this pull request Dec 20, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working OpenCL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants