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

Fixes for compilations using single/triple/quadruple precision #3910

Merged
merged 9 commits into from
Aug 18, 2024

Conversation

nmnobre
Copy link
Member

@nmnobre nmnobre commented Jul 30, 2024

Hey,

This completely fixes single/triple precision compilations for me.

Compilations using quadruple precision, however, will still fail:

  1. The boost installation we ship with libMesh is missing some necessary definitions:
    ./include/libmesh/point.h:104:73: error: use of deleted function ‘std::hash<boost::multiprecision::number<boost::multiprecision::backends::float128_backend, boost::multiprecision::et_off> >::hash()’
  2. Even with the latest version of boost, we have an incompatible assertion (__float128 is trivially copyable but boost's wrapper unfortunately isn't? After writing that, I checked, and the docs claim it should be...?! Is boost::multiprecision::float128 a trivial type? boostorg/multiprecision#635):
    ../src/geom/point.C:28:80: error: static assertion failed: Someone made Point non-TriviallyCopyable

Cheers,
-Nuno

@jwpeterson
Copy link
Member

jwpeterson commented Jul 30, 2024

@roystgnr Is the static_assert there essential to have? If not, maybe we could just ifdef it out for higher-precision builds.

@nmnobre
Copy link
Member Author

nmnobre commented Jul 30, 2024

Is the static_assert there essential to have? If not, maybe we could just ifdef it out for higher-precision builds.

I don't know why it's there. :)

@moosebuild
Copy link

moosebuild commented Jul 30, 2024

Job Coverage on f907d55 wanted to post the following:

Coverage

Inconsistent report tags were found between the head and base reports.
This can happen when reports are missing from either the head or the base.

Inconsistent tags:
32bit-np2_threads2
Full coverage report

This comment will be updated on new commits.

@roystgnr
Copy link
Member

roystgnr commented Aug 1, 2024

The reason why it's there is discussed (in rambling fashion) in #3157

Basically: in TIMPI we're copying certain things into buffers via reinterpret_cast<char*> followed by memcpy, and we just test for a StandardType<Thing> to see if we're allowed to do that, so we don't want to accidentally define StandardType<Thing<T>> unless T is trivially copyable.

Unfortunately, a T can be trivially copyable without is_trivially_copyable<T>::value==true, because there are loads of ways (almost any way) to write a constructor that can convince C++ that something non-trivial is going on without actually breaking memcpy. Sounds like Boost is hitting one of them now? If we're sure this is an overzealous assert then we can remove it; I just wish there was a way to assert what we actually want to assert here.

@nmnobre nmnobre force-pushed the fixes branch 8 times, most recently from 58c10d3 to 556ddf4 Compare August 5, 2024 10:21
@nmnobre
Copy link
Member Author

nmnobre commented Aug 5, 2024

Sounds like Boost is hitting one of them now?

Yep. Assuming I can get boostorg/multiprecision#636 merged, the newest commit, 3e80340, fixes compilations using quadruple precision if the user disables both fparser (linking issue 'cause fparser doesn't know about boost's float128, so it never instantiates anything for that type) and MetaPhysicL (because we check at points if the scalar type is built-in, and it obviously isn't marked as such https://github.com/libMesh/MetaPhysicL/blob/e67ec423140498ed65e5824c4de0f9f7143bbbdc/src/core/include/metaphysicl/compare_types.h#L163-L177).

Other than that, I think I broke CIVET by force pushing so much, so if you could give it a nudge @jwpeterson I think we should be good.

Who knows, the user might have set something there.
@roystgnr
Copy link
Member

Could you cherry-pick my last two commits from the roystgnr/fixes branch? I don't have permission to push to yours. That'll fix a minor issue I noticed and hopefully also wake up Civet.

@nmnobre
Copy link
Member Author

nmnobre commented Aug 17, 2024

Could you cherry-pick my last two commits from the roystgnr/fixes branch? I don't have permission to push to yours. That'll fix a minor issue I noticed and hopefully also wake up Civet.

Thanks, well noticed. :)

I just realised, what does the "Test 32bit" actually do?

@roystgnr roystgnr merged commit dec65e9 into libMesh:devel Aug 18, 2024
20 checks passed
@roystgnr
Copy link
Member

Most of our tests get run with 64-bit integers; that one with 32-bit. It's also one of the places where we run tests in dbg mode.

@nmnobre nmnobre deleted the fixes branch August 19, 2024 08:34
@nmnobre
Copy link
Member Author

nmnobre commented Aug 19, 2024

Thanks, I see, you change id size (dofs).

@roystgnr
Copy link
Member

We really ought to be testing the variations on subdomain_id_type and boundary_id_type too ... IIRC at least one of those has been bumped up by users recently, and they're easier to test since they don't require any changes in PETSc for full coverage.

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.

4 participants