Page MenuHomePhabricator

[OpenMP] RFC: fixes for LIBOMP_OMP_VERSION=4.5/4
ClosedPublic

Authored by lebedev.ri on Dec 9 2018, 7:26 AM.

Details

Summary

I have discovered this because i wanted to experiment with building static libomp
(with openmp-4.0 support only) for debugging purposes.

This is marked as RFC because i'm not sure if non-default values for
LIBOMP_OMP_VERSION are supposed to work or not.

There are three kinds of problems here:

  1. __kmp_compare_and_store_acq() simply does not exist. It was added in D47903 by @jlpeyton I'm guessing __kmp_atomic_compare_store_acq() was meant.
  2. In __kmp_is_ticket_lock_initialized(), lck->lk.initialized is std::atomic<bool>, while lck is kmp_ticket_lock_t *. Naturally, they can't be equality-compared. Either, it should return the value read from lck->lk.initialized, or do what __kmp_is_queuing_lock_initialized() does, compare the passed pointer with the field in the struct pointed by the pointer. I think the latter is correct-er choice here.
  3. Tests were not versioned. They assume that LIBOMP_OMP_VERSION is at the latest version.

Thoughts?

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Dec 9 2018, 7:26 AM

I think there was a discussion in the past whether it makes sense to allow building for an older version of the standard, given that the library guarantees backwards compatibility. Your findings once again show that nobody is even compile-testing the old code, so I think it would be best to just remove all the #ifdefs and code that is not needed anymore. For everything else there's a version control system that you can ask about old versions of the code.

II think it would be best to just remove all the #ifdefs and code that is not needed anymore.

Sure, i could do that instead, if that is the final decision.
I just don't want it to stay in the current sudo-broken state.

AndreyChurbanov accepted this revision.Dec 11 2018, 8:48 AM

II think it would be best to just remove all the #ifdefs and code that is not needed anymore.

Sure, i could do that instead, if that is the final decision.
I just don't want it to stay in the current sudo-broken state.

I don't mind removing the OpenMP versioning from sources, but this is much bigger change which will be harder to review.
Feel free to commit this for now, or work on the removing of all OpenMP versioning.

This revision is now accepted and ready to land.Dec 11 2018, 8:48 AM

II think it would be best to just remove all the #ifdefs and code that is not needed anymore.

Sure, i could do that instead, if that is the final decision.
I just don't want it to stay in the current sudo-broken state.

I don't mind removing the OpenMP versioning from sources, but this is much bigger change which will be harder to review.

... and the one that would need an RFC i would imagine.

Feel free to commit this for now, or work on the removing of all OpenMP versioning.

Ok, thank you for the review.

Closed by commit rL349260: [OpenMP] Fixes for LIBOMP_OMP_VERSION=45/40 (authored by lebedevri, committed by ). · Explain WhyDec 15 2018, 1:26 AM
This revision was automatically updated to reflect the committed changes.