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

Cuda safety in several headers #259

Merged
merged 1 commit into from
May 31, 2022

Conversation

lgritz
Copy link
Contributor

@lgritz lgritz commented May 18, 2022

  • Hide x86 intrinsics from Cuda compilation (half.h) that is a hard
    error when compiling for Cuda.

  • Avoid warnings about host/device decorations for defaulted
    destructors for Color3, Color4, Euler, Quat, and Shear classes -- if
    you define the destructor as = default, you shouldn't also add the
    IMATH_HOSTDEVICE decorator.

Signed-off-by: Larry Gritz [email protected]

@lgritz
Copy link
Contributor Author

lgritz commented May 18, 2022

Would appreciate a backport if/when we release a 3.1.6.

Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

Just getting to this. All looks fine, except that there's also IMATH_HOSTDEVICE on ~Quat and ~Euler. Is there a reason you skipped those? Presumably you just don't use those classes? Could you fix those while you're at it? I'm happy to submit a separate PR if you'd prefer.

And the PR title says half.h but actually it's for several classes, right?

@lgritz
Copy link
Contributor Author

lgritz commented May 31, 2022

Yeah, I will fix the other as well.

* Hide x86 intrinsics from Cuda compilation (half.h) that is a hard
  error when compiling for Cuda.

* Avoid warnings about host/device decorations for defaulted
  destructors for Color3, Color4, Euler, Quat, and Shear classes -- if
  you define the destructor as `= default`, you shouldn't also add the
  IMATH_HOSTDEVICE decorator.

Signed-off-by: Larry Gritz <[email protected]>
@lgritz lgritz changed the title Cuda safety for half.h Cuda safety in several headers May 31, 2022
@lgritz
Copy link
Contributor Author

lgritz commented May 31, 2022

Fixed as requested. New description:

Cuda safety in several headers

  • Hide x86 intrinsics from Cuda compilation (half.h) that is a hard
    error when compiling for Cuda.

  • Avoid warnings about host/device decorations for defaulted
    destructors for Color3, Color4, Euler, Quat, and Shear classes -- if
    you define the destructor as = default, you shouldn't also add the
    IMATH_HOSTDEVICE decorator.

@lgritz
Copy link
Contributor Author

lgritz commented May 31, 2022

Sorry, as you suspected, the first time around I only fixed the classes where I had encountered actual warnings (because I use them directly, or at least transitively in headers I needed to include in Cuda code).

@cary-ilm cary-ilm merged commit 49ac6aa into AcademySoftwareFoundation:main May 31, 2022
@lgritz lgritz deleted the lg-cudasafety branch August 4, 2022 17:16
cary-ilm pushed a commit to cary-ilm/Imath that referenced this pull request Oct 31, 2022
* Hide x86 intrinsics from Cuda compilation (half.h) that is a hard
  error when compiling for Cuda.

* Avoid warnings about host/device decorations for defaulted
  destructors for Color3, Color4, Euler, Quat, and Shear classes -- if
  you define the destructor as `= default`, you shouldn't also add the
  IMATH_HOSTDEVICE decorator.

Signed-off-by: Larry Gritz <[email protected]>
cary-ilm pushed a commit that referenced this pull request Nov 3, 2022
* Hide x86 intrinsics from Cuda compilation (half.h) that is a hard
  error when compiling for Cuda.

* Avoid warnings about host/device decorations for defaulted
  destructors for Color3, Color4, Euler, Quat, and Shear classes -- if
  you define the destructor as `= default`, you shouldn't also add the
  IMATH_HOSTDEVICE decorator.

Signed-off-by: Larry Gritz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants