lanl / Draco

An object-oriented component library supporting radiation transport applications.
Other
55 stars 44 forks source link

Make basic CDI functions constexpr #1109

Closed alexrlongne closed 3 years ago

alexrlongne commented 3 years ago

Background

Purpose of Pull Request

Description of changes

Status

KineticTheory commented 3 years ago

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

alexrlongne commented 3 years ago

@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.

attom commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

kgbudge commented 3 years ago

[Z]

From: "Kelly (KT) Thompson" @.> Reply-To: lanl/Draco @.> Date: Wednesday, August 18, 2021 at 8:22 AM To: lanl/Draco @.> Cc: Subscribed @.> Subject: [EXTERNAL] Re: [lanl/Draco] WIP: Make basic CDI functions constexpr (#1109)

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

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/lanl/Draco/pull/1109#issuecomment-901157668, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AE4AAATPD7AQO3JPD55GNZTT5O6ZZANCNFSM5CIAVTHA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email.

alexrlongne commented 3 years ago

@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 commented 3 years ago

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__.

codecov[bot] commented 3 years ago

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 commented 3 years ago

This should be ready for review again.