OpenCL-Update #1337
OpenCL-Update #1337
Conversation
There was a problem hiding this comment.
This PR is quite massive so I didn't have time to make a complete review of this PR.
But I think it's better to have a partial review than no review at all.
In this review I basically only looked through CL.cs and commented on what caught my eye.
I'm not a super big fan of all of the "property" classes, but I've not looked at them in much detail and I'm not sure what a good alternate solution would be.
There is a lot of review that should be done on this quite massive PR.
Stuff like making sure all of the types are correct according to the OpenCL spec and c calling conventions. (which is already hard because there is no 1:1 c# equivalent to size_t).
Just ping me if you want another review.
src/OpenTK.Compute/OpenCL/CL.cs
Outdated
| public static CLResultCode SupportsPlatformExtension( | ||
| this CLPlatform platform, | ||
| string extension, | ||
| out bool extensionSupported) | ||
| { | ||
| extensionSupported = false; | ||
| var resultCode = GetPlatformInfo(platform, PlatformInfo.Extensions, out byte[] bytes); | ||
| var extensions = Encoding.ASCII.GetString(bytes).Split(" "); | ||
| foreach (var supportedExtension in extensions) | ||
| { | ||
| if (supportedExtension == extension) | ||
| extensionSupported = true; | ||
| } | ||
|
|
||
| return resultCode; | ||
| } |
There was a problem hiding this comment.
This function is working at a higher level than it should and it's also making API transformations that are not ideal.
The OpenCL api allows you to call clGetPlatformInfo once and get a string of all extensions, while this function limits the user to querying only one extension at a time.
This changes the API we expose compared to OpenCL which isn't great.
Comparing text is also something we ideally don't want to do either.
If this needs to function then I think it should be something like this:
public static CLResultCode GetPlatformExtensions(CLPlatform platform, out string extensions)But ideally we shouldn't be doing that.
We should just have a version of GetPlatformInfo that has an out string out parameter ( which will break typesafety for the enums if we don't put the string returning enums in their own type, like we've done for OpenAL ).
There was a problem hiding this comment.
Doesn't fit with the "pure" implementation of the cl bindings so has been removed
src/OpenTK.Compute/OpenCL/CL.cs
Outdated
| var resultCode = CreateSubDevices(inDevice, properties.CreatePropertyArray(), 0, null, out uint sizeReturned); | ||
| outDevices = new CLDevice[sizeReturned]; | ||
| if (sizeReturned == 0) | ||
| return resultCode; | ||
| return CreateSubDevices(inDevice, properties.CreatePropertyArray(), sizeReturned, outDevices, out _); |
There was a problem hiding this comment.
Here we are calling properties.CreatePropertyArray() twice which is unnecessary, the result of the first call should be saved and reused for the second call.
src/OpenTK.Compute/OpenCL/CL.cs
Outdated
| IntPtr[] sourceList = source.Select(s => Marshal.StringToHGlobalAnsi(s)).ToArray(); | ||
| nuint[] sourceLengths = source.Select(s => (nuint)s.Length).ToArray(); |
There was a problem hiding this comment.
This should be done with a simple for-loop. No need for linq.
This function should also be responsible for freeing the strings allocated.
This should also probably be done automatically by pinvoke by just doing:
[DllImport(LibName, CallingConvention = CallingConvention, EntryPoint = "clCreateProgramWithSource")]
public static extern CLProgram CreateProgramWithSource(
[In] CLContext context,
[In] nuint size,
[In] string[] strings,
[In] nuint[] lengths,
[Out] out CLResultCode resultCode);and potentially specifying a character encoding in the DllImport.
And we likely also want to provide a int[] for the functions that take a nuint[] atm, as most people will be working with ints rather than nuints. But that is pretty low priority, nothing to hold this PR back for.
There was a problem hiding this comment.
nuint is the c# equivalent of size_t, behind the scenes it's a uint64 on 64 bit OS and 32 on 32 bit.
There was a problem hiding this comment.
nuint and size_t are not guaranteed to be equal. They most likely are for every machine where you can get c# run Ing on. But they are not equivalent types in terms of the ABI.
Nuint is the only option for c#/c interop with code that uses size_t and it's going to work for like 99% of cases so it doesn't really matter.
src/OpenTK.Compute/OpenCL/CL.cs
Outdated
| if (specValue is bool) | ||
| { | ||
| ushort shortVal = (bool)Convert.ChangeType(specValue, typeof(bool)) ? (ushort)1 : (ushort)0; | ||
| ushort* shortArg = &shortVal; | ||
| return SetProgramSpecializationConstant(program, specId, (nuint)sizeof(ushort), (IntPtr)shortArg); | ||
| } |
There was a problem hiding this comment.
This should really be a separate overload for bool and not a special case here (if we are even going to have a bool overload. Does OpenCL have a bool type?).
And it should be using pattern matching (if (specValue is bool specBool)) if this is to stay.
There was a problem hiding this comment.
In the documentation, it states that booleans should have a size of 1. I've updated the code to better fit this
spec_size specifies the size in bytes of the data pointed to by spec_value. This should be 1 for boolean constants. For all other constant types this should match the size of the specialization constant in the module.
…ns, tidied up with regards to MR comments
| var specSize = (nuint)sizeof(T); | ||
| if (specValue is bool) | ||
| specSize = 1; |
There was a problem hiding this comment.
sizeof(bool) is equal to 1 in C# so we don't need this check at all actually.
NogginBops
force-pushed
the
opentk5.0
branch
2 times, most recently
from
416a63a
to
b57b01c
Compare
|
Ill try and improve the documentation while you are doing this restructuring as well |
Purpose of this PR
Testing status
Comments