Skip to content
This repository was archived by the owner on Aug 11, 2023. It is now read-only.

Fix Mandelbrot demo for hardware which doesn't support doubles #259

Merged
merged 8 commits into from
Jun 23, 2021

Conversation

martinstarkov
Copy link
Collaborator

@martinstarkov martinstarkov commented Jun 14, 2021

After messing with templates and trying to make the demo work with both floats and doubles depending on hardware, I noted that this overcomplicates the demo and makes it harder for beginners to focus on the point of the demo (which is to showcase a SYCL application). Floats work on all hardware hence changing doubles to floats makes the demo more general (without overcomplicating the source files).

@martinstarkov martinstarkov changed the title Fix Mandelbrot demo for hardware which doesn Fix Mandelbrot demo for hardware which doesn't support doubles Jun 14, 2021
@DuncanMcBain
Copy link
Member

The code you've changed here is entirely host-side code. Which hardware doesn't support double on the host CPU? Aside from that, I'd be pretty disappointed if it were so hard for us to have a demo which can run in both single- and double-precision modes. Surely the changes aren't that drastic?

@martinstarkov
Copy link
Collaborator Author

martinstarkov commented Jun 14, 2021

The code you've changed here is entirely host-side code. Which hardware doesn't support double on the host CPU? Aside from that, I'd be pretty disappointed if it were so hard for us to have a demo which can run in both single- and double-precision modes. Surely the changes aren't that drastic?

The code I changed modifies the device kernel as the class template takes in a float which will change the kernel too. My Intel(R) Core(TM) i5-5200U CPU drivers for OpenCL do not support double floating-point precision and I'm sure there are other examples. I think you are correct that perhaps the demo should support both but from my attempts it has been very difficult to get it to work since the source files need to be separated with the float/double instantiation but are instantiated already if you call them in an if-statement (checking if device supports double floating point precision). I'm not sure how to resolve this issue and I honestly feel like the demo will lose a lot of clarity if we have two source files. People in the SYCL-ECO chat were not able to help me when I was attempting a fix so I figured this would be simpler and works. The demo still runs well with floats and the user can zoom in for a very reasonable amount.

If you have any suggestions to support something like this (pseudo code) then please do let me know:

void calc() {
   if (device.get_info<???>()  == ???) {
      internal_calc<double>();
   } else {
      internal_calc<float>();
  }
}

I was not able to figure out how to separate the template instantiation into two source files while having the above call be in one file..

- Removed MandelbrotCalculator class templating
- Switched to using doubles for internal boundaries for maximum precision
- Added variable and getter function for SYCL devices which support doubles
- Templated the calc and internal_calc functions instead of the class
@martinstarkov
Copy link
Collaborator Author

Made the changes requested by @DuncanMcBain
I believe my approach to checking for double support is sufficient based on SYCL 2020 spec.

Copy link
Collaborator Author

@martinstarkov martinstarkov left a comment

Choose a reason for hiding this comment

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

The title of 631cfc3 commit should be "Change floats to doubles and check for SYCL device floating-point support" Got my words mixed up there.

Copy link
Member

@DuncanMcBain DuncanMcBain left a comment

Choose a reason for hiding this comment

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

looks good!

@martinstarkov
Copy link
Collaborator Author

Made all the requested changes

@DuncanMcBain DuncanMcBain merged commit 2069321 into codeplaysoftware:master Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants