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

Make basic CDI functions constexpr #1109

Merged
merged 4 commits into from
Aug 19, 2021
Merged

Conversation

alexrlongne
Copy link
Contributor

@alexrlongne alexrlongne commented Aug 16, 2021

Background

  • It would be nice to have a clear delineation of which CDI functions (static or otherwise) use class data. This PR just moves the lowest level functions that don't use the static member variable frequencyGroupBoundaries. I've also marked the low-level functions constexpr do that the GPU can call them more easily.

Purpose of Pull Request

  • Make it easier to call simple Planck integration functions.

Description of changes

  • Move functions that don't require any class member data outside CDI class
  • Mark basic integration functions as constexpr
  • Remove files that formerly held low-level planck integration functions

Status

+ Move functions that don't require any class member data outside CDI class
+ Mark basic integration functions as constexpr
+ Remove files that formerly held low-level planck integration functions
@KineticTheory
Copy link
Collaborator

@alexrlongne It is interesting that LLVM accepts your code, but gcc and msvc fail with the same error, expression not constexpr.

@alexrlongne
Copy link
Contributor Author

@KineticTheory Ahhh ok, I was curious if this would happen. Looks like the "data" function of std::array is not constexpr to those other compilers. I'll see if I can rework this.

@alexrlongne alexrlongne changed the title Make basic CDI functions constexpr WIP: Make basic CDI functions constexpr Aug 16, 2021
@attom
Copy link
Contributor

attom commented Aug 16, 2021

Will this change the function signatures in such a way that it will break downstream codes? (Isn't a problem; I'd just like a heads-up and @jhchang-lanl would probably too :) )

@alexrlongne
Copy link
Contributor Author

@attom Yeah, it might, sorry, I should have marked this WIP right at submission. I'll do a quick grep through your repos in a minute.

@alexrlongne
Copy link
Contributor Author

OK! I think I fixed the constexpr issue. It's something to look again when we go to C++17.

@attom I have a branch with the fixes for Capsaicin. A few functions live in the rtt_cdi namespace now, the rest of the API is the same. I'll push that branch to trt in a moment.

@KineticTheory
Copy link
Collaborator

@alexrlongne Don't you wish there were some language standards that compiler's would adhere to?
Oh, wait, we have those. 😃

@kgbudge
Copy link
Contributor

kgbudge commented Aug 18, 2021 via email

@alexrlongne
Copy link
Contributor Author

@KineticTheory hahah, right? Stack Overflow that it's actually MSVC that's correct here and that GCC provides a "built-in" constexpr version (indeed cppreference says there's not a constexpr std::pow).

@alexrlongne
Copy link
Contributor Author

Whoops, looks like MSVC also doesn't do constexpr exp or whatever it uses for the FMA. I'll back off on the constexpr and just mark them with the the macro we have for __host__ __device__.

+ Hard code std::pow of max double as there's no constexpr pow in msvc
+ Add device/config to Cmake for CDI
@codecov
Copy link

codecov bot commented Aug 18, 2021

Codecov Report

Merging #1109 (60200f6) into develop (3c93de8) will increase coverage by 0.0%.
The diff coverage is 98.6%.

@@           Coverage Diff           @@
##           develop   #1109   +/-   ##
=======================================
  Coverage     88.9%   89.0%           
=======================================
  Files          376     373    -3     
  Lines        19569   19573    +4     
=======================================
+ Hits         17416   17420    +4     
  Misses        2153    2153           

@alexrlongne alexrlongne changed the title WIP: Make basic CDI functions constexpr Make basic CDI functions constexpr Aug 19, 2021
@alexrlongne
Copy link
Contributor Author

This should be ready for review again.

@KineticTheory KineticTheory added this to the Draco-7_12_0 milestone Aug 19, 2021
@KineticTheory KineticTheory merged commit 605e1e1 into lanl:develop Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants