This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] remove usage of std::abs in the new loop collapse support code
ClosedPublic

Authored by vadikp-intel on May 18 2023, 9:40 AM.

Details

Summary

on some platforms, std::abs may inadvertently pull in a math library. This patch replaces its use in the new loop collapse code with a no thrills in-situ implementation.

Diff Detail

Event Timeline

vadikp-intel created this revision.May 18 2023, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 9:40 AM
vadikp-intel requested review of this revision.May 18 2023, 9:40 AM

Could you also check all the uses and replace them all at once? After that, can you remove the <cmath> header as well?

tianshilei1992 added inline comments.May 18 2023, 9:43 AM
openmp/runtime/src/kmp_collapse.cpp
29

_X is not in any format that we try to follow here. Neither LLVM nor libomp standard.

29

Potentially it can be called something like __kmp_abs.

removed <cmath> reference and changed the names of the local replacement implementations to convention.

I have reworked the naming but am not seeing any other uses of std::abs in the runtime code. There are a couple of <cmath> references, but none of them appear to be reachable in the current build: one is apparently in a test case, the other - in the stats collection support which is off by default and where math use is heavier (e.g. sqrt) making its removal trickier.

vadikp-intel marked 2 inline comments as done.May 18 2023, 10:32 AM

openmp/runtime/src/kmp_stats.cpp contains a std::fabs. Can you update it as well? Thanks.

Changing kmp_stats to use the new definitions would require moving them up into a header somewhere while still requiring <cmath> in kmp_stats. Do you think this would be worth it?

tianshilei1992 accepted this revision.May 18 2023, 11:07 AM

Sure, then we can come back to it later. We might want to move those things into kmp.h or something like kmp_math.h eventually such that all source files can use them.

This revision is now accepted and ready to land.May 18 2023, 11:07 AM