remove `KernelBundle`, change signature of `[get|is]ValidWorkDiv*` #2349

psychocoderHPC · 2024-08-09T14:02:22Z

revert introduced KernelBundle from PR Fix getValidWorkDiv for CUDA and ROCm [I] #2222 #2251
change signature of getValidWorkDivForKernel,isValidWorkDivKernel and isValidWorkDivKernel
- isValidWorkDiv is using the work division as first parameter and all dependencies e.g. device, kernel, ... followed
reuse old naming getValidWorkDiv and isValidWorkDiv
use new interface for getValidWorkDiv in examples and tests

This PR replaces #2340 where @fwyzard observed performance reduction #2340 (comment) and #2339 (comment)

Review

this PR is split into two commits, first changing the interface and second usage of the interface

psychocoderHPC · 2024-08-09T14:07:02Z

example/convolution2D/src/convolution2D.cpp

        convolutionKernel2D,
-        alpaka::getPtrNative(bufInputAcc),
-        alpaka::getPtrNative(outputDeviceMemory),
+        std::data(bufInputAcc),


std::data is used for the kernel call too.

include/alpaka/workdiv/WorkDivHelpers.hpp

fwyzard · 2024-08-13T09:07:50Z

include/alpaka/kernel/TaskKernelGpuUniformCudaHipRt.hpp

-                -> alpaka::KernelFunctionAttributes
+                [[maybe_unused]] TDev const& dev,
+                [[maybe_unused]] TKernelFn const& kernelFn,
+                [[maybe_unused]] TArgs&&... args) -> alpaka::KernelFunctionAttributes


@psychocoderHPC , irrespective if we are using the kernel bundle, what happens if the arguments passed to getFunctionAttributes are convertible to the ones expected by the alpaka kernel, but do not match exactly ?

For example, if the alpaka kernel takes (TAcc const& acc, float x, float y, float z) and we call getFunctionAttributes with attributes 1.0, 2.0, 3.0 , i.e. double, not float.

Does it still work correctly ?

We do not have the kernel bundle anymore in this PR.
But the behavior is equal to the behavior before, I would say it is implicit casted because it follows the C*+ rules, but I am not 100% sure.

The evaluated function attributes should be correct because we use the same function signature we use for the kernel call. I do not see that this can be different.

Ah, I see... the actual kernel is gpuKernel; the arguments are passed as whatever they are to gpuKernel, and it's only inside gpuKernel that we call kernelFn and potentially convert the arguments.

fwyzard · 2024-08-13T14:07:29Z

include/alpaka/workdiv/WorkDivHelpers.hpp

    //! \return If the work division is valid for the given accelerator device properties.
-    template<typename TDim, typename TIdx, typename TWorkDiv>
-    ALPAKA_FN_HOST auto isValidWorkDiv(AccDevProps<TDim, TIdx> const& accDevProps, TWorkDiv const& workDiv) -> bool
+    template<, typename TWorkDiv, typename TDim, typename TIdx>


Suggested change

template<, typename TWorkDiv, typename TDim, typename TIdx>

template<typename TWorkDiv, typename TDim, typename TIdx>

?

type during the last refactroing of the template argument order :-(

- revert introduced `KernelBundle` in alpaka-group#2251 - change signature of `getValidWorkDivForKernel`,`isValidWorkDivKernel` and `isValidWorkDivKernel` - reuse old naming `getValidWorkDiv` and `isValidWorkDiv`

psychocoderHPC · 2024-08-14T11:56:39Z

@fwyzard ready to be merged

docs/source/basic/cheatsheet.rst

fwyzard · 2024-08-14T12:30:20Z

example/bufferCopy/src/bufferCopy.cpp

@@ -118,7 +118,7 @@ auto example(TAccTag const&) -> int
    using Data = std::uint32_t;
    constexpr Idx nElementsPerDim = 2;

-    const Vec extents(Vec::all(static_cast<Idx>(nElementsPerDim)));
+    Vec const extents(Vec::all(static_cast<Idx>(nElementsPerDim)));


Suggested change

Vec const extents(Vec::all(static_cast<Idx>(nElementsPerDim)));

Vec const extents(Vec::all(nElementsPerDim));

since nElementsPerDim is already of type Idx.

fwyzard · 2024-08-14T13:01:42Z

include/alpaka/workdiv/WorkDivHelpers.hpp

+        typename... TArgs>
+    ALPAKA_FN_HOST auto getValidWorkDiv(
+        KernelCfg<TAcc, TGridElemExtent, TThreadElemExtent> const& kernelCfg,
+        [[maybe_unused]] TDev const& dev,


why [[maybe_unused]] ?

fwyzard · 2024-08-14T13:08:17Z

docs/source/basic/cheatsheet.rst

-       bundeledKernel,
-       globalThreadExtent, elementsPerThread,
+     KernelCfg<Acc> const kernelCfg = {
+       globalThreadExtent,


This should be something like globalElementsExtent rather than globalThreadExtent, because the number of elements per thread are also taken into account.

fwyzard · 2024-08-14T13:09:50Z

example/bufferCopy/src/bufferCopy.cpp

-    auto const& bundeledFillBufferKernel = alpaka::KernelBundle(fillBufferKernel, hostViewPlainPtrMdSpan);
-    auto const hostWorkDiv
-        = alpaka::getValidWorkDivForKernel<Host>(devHost, bundeledFillBufferKernel, threadsPerGrid, elementsPerThread);
+    alpaka::KernelCfg<Host> const hostKernelCfg = {threadsPerGrid, elementsPerThread};


Suggested change

alpaka::KernelCfg<Host> const hostKernelCfg = {threadsPerGrid, elementsPerThread};

alpaka::KernelCfg<Host> const hostKernelCfg = {elementsPerGrid, elementsPerThread};

?

The same comment applies to the other examples.

fwyzard · 2024-08-14T13:11:44Z

@psychocoderHPC I've left some comments about possible improvements and suggestions.

We can also merge the PR as-is, and address them later, if you prefer.

psychocoderHPC · 2024-08-14T15:09:45Z

@psychocoderHPC I've left some comments about possible improvements and suggestions.

We can also merge the PR as-is, and address them later, if you prefer.

I will fix only #2349 (comment) because it is a bug. All other are copy past from the existing code. I can address this in an seperate PR.

psychocoderHPC · 2024-08-14T15:11:59Z

@fwyzard I pushed the cheat sheet fix

psychocoderHPC added the Type:Refactoring label Aug 9, 2024

psychocoderHPC added this to the 1.2.0 milestone Aug 9, 2024

psychocoderHPC requested a review from fwyzard August 9, 2024 14:02

psychocoderHPC commented Aug 9, 2024

View reviewed changes

psychocoderHPC force-pushed the topic-getValidWorkdivRefactoring branch 3 times, most recently from 246f4a8 to 527b4bc Compare August 9, 2024 14:48

psychocoderHPC changed the title ~~Topic get valid workdiv refactoring~~ remove KernelBundle, change signature of getValidWorkDiv* Aug 9, 2024

psychocoderHPC force-pushed the topic-getValidWorkdivRefactoring branch 5 times, most recently from a3b09c9 to 2ad95ea Compare August 9, 2024 16:06

psychocoderHPC mentioned this pull request Aug 9, 2024

[WIP] Add CloverLeaf benchmark #2316

Draft

1 task

psychocoderHPC changed the title ~~remove KernelBundle, change signature of getValidWorkDiv*~~ remove KernelBundle, change signature of [get|is]ValidWorkDiv* Aug 9, 2024

psychocoderHPC force-pushed the topic-getValidWorkdivRefactoring branch from 2ad95ea to 0ebaa66 Compare August 9, 2024 16:30

chillenzer reviewed Aug 13, 2024

View reviewed changes

include/alpaka/workdiv/WorkDivHelpers.hpp Outdated Show resolved Hide resolved

fwyzard reviewed Aug 13, 2024

View reviewed changes

psychocoderHPC force-pushed the topic-getValidWorkdivRefactoring branch from 0ebaa66 to 88e40a6 Compare August 13, 2024 11:45

fwyzard reviewed Aug 13, 2024

View reviewed changes

psychocoderHPC added 2 commits August 14, 2024 09:59

remove KernelBundle, change signature of getValidWorkDiv*

cbf3f6c

- revert introduced `KernelBundle` in alpaka-group#2251 - change signature of `getValidWorkDivForKernel`,`isValidWorkDivKernel` and `isValidWorkDivKernel` - reuse old naming `getValidWorkDiv` and `isValidWorkDiv`

use new interface for getValidWorkDiv

0ec7f32

psychocoderHPC force-pushed the topic-getValidWorkdivRefactoring branch from 88e40a6 to 0ec7f32 Compare August 14, 2024 07:59

fwyzard reviewed Aug 14, 2024

View reviewed changes

docs/source/basic/cheatsheet.rst Outdated Show resolved Hide resolved

fwyzard reviewed Aug 14, 2024

View reviewed changes

fwyzard previously approved these changes Aug 14, 2024

View reviewed changes

fix cheat sheet

f453c9b

psychocoderHPC dismissed fwyzard’s stale review via f453c9b August 14, 2024 15:11

fwyzard approved these changes Aug 14, 2024

View reviewed changes

SimeonEhrig merged commit d7c459d into alpaka-group:develop Aug 15, 2024
22 checks passed

This was referenced Aug 16, 2024

refactor examples/tests + additional alpaka::exec() interface #2340

Closed

Use KernelBundle in exec and task create [KB-III] #2318

Closed

psychocoderHPC deleted the topic-getValidWorkdivRefactoring branch August 16, 2024 12:13

psychocoderHPC mentioned this pull request Aug 16, 2024

rename threadsPerGrid to elementsPerGrid #2362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove `KernelBundle`, change signature of `[get|is]ValidWorkDiv*` #2349

remove `KernelBundle`, change signature of `[get|is]ValidWorkDiv*` #2349

psychocoderHPC commented Aug 9, 2024 •

edited

Loading

psychocoderHPC Aug 9, 2024

fwyzard Aug 13, 2024

psychocoderHPC Aug 13, 2024

psychocoderHPC Aug 13, 2024 •

edited

Loading

fwyzard Aug 14, 2024

fwyzard Aug 13, 2024

psychocoderHPC Aug 14, 2024

psychocoderHPC Aug 14, 2024

psychocoderHPC commented Aug 14, 2024

fwyzard Aug 14, 2024

fwyzard Aug 14, 2024

fwyzard Aug 14, 2024

fwyzard Aug 14, 2024

fwyzard Aug 14, 2024

fwyzard commented Aug 14, 2024

psychocoderHPC commented Aug 14, 2024

psychocoderHPC commented Aug 14, 2024

	template<, typename TWorkDiv, typename TDim, typename TIdx>
	template<typename TWorkDiv, typename TDim, typename TIdx>

	Vec const extents(Vec::all(static_cast<Idx>(nElementsPerDim)));
	Vec const extents(Vec::all(nElementsPerDim));

	alpaka::KernelCfg<Host> const hostKernelCfg = {threadsPerGrid, elementsPerThread};
	alpaka::KernelCfg<Host> const hostKernelCfg = {elementsPerGrid, elementsPerThread};

remove KernelBundle, change signature of [get|is]ValidWorkDiv* #2349

remove KernelBundle, change signature of [get|is]ValidWorkDiv* #2349

Conversation

psychocoderHPC commented Aug 9, 2024 • edited Loading

Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psychocoderHPC Aug 13, 2024 • edited Loading

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

psychocoderHPC commented Aug 14, 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

fwyzard commented Aug 14, 2024

psychocoderHPC commented Aug 14, 2024

psychocoderHPC commented Aug 14, 2024

remove `KernelBundle`, change signature of `[get|is]ValidWorkDiv*` #2349

remove `KernelBundle`, change signature of `[get|is]ValidWorkDiv*` #2349

psychocoderHPC commented Aug 9, 2024 •

edited

Loading

psychocoderHPC Aug 13, 2024 •

edited

Loading