This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP]Fix trivial build failure in MacOS
ClosedPublic

Authored by Leporacanthicus on May 16 2023, 9:19 AM.

Details

Summary

MacOS build of LLVM with OpenMP enabled fails with an error
that it doesn't know what std::abs is. Fix by including <cmath>
so that the relevant function declaration is included.

No functional change intended.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 9:19 AM
Leporacanthicus requested review of this revision.May 16 2023, 9:19 AM
Herald added a project: Restricted Project. · View Herald Transcript

Hmm, I'm not sure if we want to include <cmath> here because libomp definitely doesn't depend on libm. We might want to deal with it in https://reviews.llvm.org/D149010 instead.

Hmm, I'm not sure if we want to include <cmath> here because libomp definitely doesn't depend on libm. We might want to deal with it in https://reviews.llvm.org/D149010 instead.

I guess we could include cstdlib instead, would that be better. If you think that's better, let me know, happy to update the patch.

The other alternative would be to "not use std::abs" - but to me that doesn't seem like a great solution. [Interestingly, the file kmp_stats.cpp, line 181 does call std::fabs, but I guess that is done inline, so it doesn't actually use libm anyway] - and includes <cmath> - maybe kmp_stats.cpp is "special" here? I'm happy for someone else to fix this - my main reason for posting this patch was to help some colleagues of mine move on - which they are now able to do. If another fix is done elsewhere, I'm happy to abandon this patch.

tianshilei1992 accepted this revision.May 17 2023, 9:21 AM

Yeah, it is fine as long as the function can be inlined. Thanks for pointing to openmp/runtime/src/kmp_stats.cpp. I didn't notice that. We generally avoid pulling dependences for libomp. I think we can go with it for now. If anything comes up in the future, we can always revisit it.

This revision is now accepted and ready to land.May 17 2023, 9:21 AM
This revision was automatically updated to reflect the committed changes.