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

Add a usecase discussion of how to deal with function pointers #577

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crtrott
Copy link
Member

@crtrott crtrott commented Sep 3, 2024

Copy link
Contributor

@JBludau JBludau left a comment

Choose a reason for hiding this comment

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

I think it is a good addition

I can use the function pointer in a host parallel_for!
foo called from host
Now I will crash if we compiled for CUDA/HIP
cudaDeviceSynchronize() error( cudaErrorIllegalAddress): an illegal memory access was encountered /home/crtrott/Kokkos/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:153
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cudaDeviceSynchronize() error( cudaErrorIllegalAddress): an illegal memory access was encountered /home/crtrott/Kokkos/kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:153
cudaDeviceSynchronize() error( cudaErrorIllegalAddress): an illegal memory access was encountered /kokkos/core/src/Cuda/Kokkos_Cuda_Instance.cpp:153

};
```

This class is templated on the function pointer type, contains a pointer for both the host and the device version, and has a templated operator that forwards all the arguments, and calls the appropriate function pointer depending on call site.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This class is templated on the function pointer type, contains a pointer for both the host and the device version, and has a templated operator that forwards all the arguments, and calls the appropriate function pointer depending on call site.
This class is templated on the function pointer type, contains a pointer for both the host and the device version, and has a templated operator that copies all the arguments, and calls the appropriate function pointer depending on the call site.

I find it kind of important to make the distinction here as it adds another drawback.

};
```

However we still need to initialize the two function pointers, using the device initialization approach from before, and also initializing the host side. Note: the order of initialization and `deep_copy` matters, because `deep_copy` will overwrite both member poitners in our wrapper class:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
However we still need to initialize the two function pointers, using the device initialization approach from before, and also initializing the host side. Note: the order of initialization and `deep_copy` matters, because `deep_copy` will overwrite both member poitners in our wrapper class:
However we still need to initialize the two function pointers using the device initialization approach from before, and also initializing the host side. Note: the order of initialization and `deep_copy` matters, because `deep_copy` will overwrite both member poitners in our wrapper class:

```

While this generally works (and note this code will also work if you simply compile for host-only) there may be architectures in the future where this fails.
Also we in principle strongly discourage the use of function pointers - the amount of possible problems you run into is fairly significant.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Also we in principle strongly discourage the use of function pointers - the amount of possible problems you run into is fairly significant.
Also we strongly discourage the use of function pointers - the amount of possible problems you run into is fairly significant.

I would just make that unconditional

Comment on lines +24 to +25
KOKKOS_IF_ON_HOST(printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(printf("foo called from device\n");)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KOKKOS_IF_ON_HOST(printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(printf("foo called from device\n");)
KOKKOS_IF_ON_HOST(Kokkos::printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(Kokkos::printf("foo called from device\n");)

Comment on lines +37 to +38
KOKKOS_IF_ON_HOST(printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(printf("foo called from device\n");)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KOKKOS_IF_ON_HOST(printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(printf("foo called from device\n");)
KOKKOS_IF_ON_HOST(Kokkos::printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(Kokkos::printf("foo called from device\n");)

Aborted (core dumped)
```

*The function pointer we created was for a host function: you can not use it on device same as you can't dereference data pointers to host data!*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*The function pointer we created was for a host function: you can not use it on device same as you can't dereference data pointers to host data!*
*The function pointer we created was for a host function: you can not use it on the device just like you can't dereference data pointers to host data!*

Kokkos::deep_copy(A, A_v);
```

We are leveraging here the fact that `deep_copy` allows you to copy from and to a host scalar value, from a `View` of rank-0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We are leveraging here the fact that `deep_copy` allows you to copy from and to a host scalar value, from a `View` of rank-0.
We are leveraging here the fact that `deep_copy` allows you to copy in both direction between a host scalar value and a `View` of rank-0.


We are leveraging here the fact that `deep_copy` allows you to copy from and to a host scalar value, from a `View` of rank-0.

If we do this `A` will contain a function pointer to a device function. That means we can capture `A` into a parallel region and execute it on the device, but now it will crash on the host.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If we do this `A` will contain a function pointer to a device function. That means we can capture `A` into a parallel region and execute it on the device, but now it will crash on the host.
If we do this, `A` will contain a function pointer to a device function. That means we can capture `A` into a parallel region and execute it on the device, but now it will crash on the host.

Comment on lines +125 to +126
KOKKOS_IF_ON_HOST(printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(printf("foo called from device\n");)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KOKKOS_IF_ON_HOST(printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(printf("foo called from device\n");)
KOKKOS_IF_ON_HOST(Kokkos::printf("foo called from host\n");)
KOKKOS_IF_ON_DEVICE(Kokkos::printf("foo called from device\n");)


## Creating a dual function pointer object

To do better than this we need something which contains both the device function pointer and the host function pointer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To do better than this we need something which contains both the device function pointer and the host function pointer.
To do better than this, we need something which contains both the device function pointer and the host function pointer.

};
```

However we still need to initialize the two function pointers, using the device initialization approach from before, and also initializing the host side. Note: the order of initialization and `deep_copy` matters, because `deep_copy` will overwrite both member poitners in our wrapper class:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
However we still need to initialize the two function pointers, using the device initialization approach from before, and also initializing the host side. Note: the order of initialization and `deep_copy` matters, because `deep_copy` will overwrite both member poitners in our wrapper class:
However, we still need to initialize the two function pointers, using the device initialization approach from before, and also initializing the host side. Note: the order of initialization and `deep_copy` matters, because `deep_copy` will overwrite both member poitners in our wrapper class:

}
```

While this generally works (and note this code will also work if you simply compile for host-only) there may be architectures in the future where this fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
While this generally works (and note this code will also work if you simply compile for host-only) there may be architectures in the future where this fails.
While this generally works (and note this code will also work if you simply compile for host-only), there may be architectures in the future where this fails. SYCL (and therefore the SYCL backend) doesn't allow using function pointers on the device for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants