This is an archive of the discontinued LLVM Phabricator instance.

[flang][runtime] Remove dependency on C++ <mutex> on Windows
ClosedPublic

Authored by mmuetzel on Jun 8 2022, 8:59 AM.

Details

Summary

The implementation of the Lock class on Windows currently uses C++ mutexes. That introduces a dependency on the C++ runtime on that platform.

Use a Windows CriticalSection instead of a std::mutex to avoid that dependency.

This works for me with MinGW (tested in a CLANG64 environment of MSYS2).

See also D126291.

This is the first time I'm uploading a patch to Phabricator. So, please let me know if this isn't the right way to propose changes.

Diff Detail

Event Timeline

mmuetzel created this revision.Jun 8 2022, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 8:59 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
mmuetzel requested review of this revision.Jun 8 2022, 8:59 AM
mmuetzel updated this revision to Diff 435245.EditedJun 8 2022, 10:35 AM

Attempt to fix compilation error with MSVC.

Looks reasonable to me, but I'll leave it up to someone else more involved in flang to approve.

This patch oversimplifies the code, making it work only for WIN32 and with pthreads, but no longer with C++ mutexes.

klausler requested changes to this revision.Jun 8 2022, 1:05 PM
This revision now requires changes to proceed.Jun 8 2022, 1:05 PM

This patch oversimplifies the code, making it work only for WIN32 and with pthreads, but no longer with C++ mutexes.

The previous logic undefined USE_PTHREADS only on Windows and defined it to 1 everywhere else.
This change proposes an alternative implementation on Windows. It would be possible to leave in the code that uses std::mutex. But that part of the code would be essentially dead, and there would be no condition under which it would be compiled.

I can do that change. Just would like to confirm that this is really what you'd like to have.

Yes, please.

mmuetzel updated this revision to Diff 435340.EditedJun 8 2022, 2:35 PM

Leave in (unreachable) code that uses std::mutex.
Ok like this?

klausler accepted this revision.Jun 8 2022, 2:38 PM
This revision is now accepted and ready to land.Jun 8 2022, 2:38 PM

Hi @mmuetzel,

Thanks for submitting this! I think the NOMINMAX trick should fix the issues I was seeing on Windows on Arm, but I still need to test to be sure (it's slow...). Do you have commit access or should I commit on your behalf?
One small Phabricator nitpick: it's nice to upload patches with context (you don't have to do that for this patch, I was just mentioning it for the future).

Hi @mmuetzel,

Thanks for submitting this! I think the NOMINMAX trick should fix the issues I was seeing on Windows on Arm, but I still need to test to be sure (it's slow...). Do you have commit access or should I commit on your behalf?

I don't think I have commit access (unless there is some phabricator magic going on behind the curtains that I'm not aware of). I'd be happy if you could push this to the repo (if/when your tests have been successful).

One small Phabricator nitpick: it's nice to upload patches with context (you don't have to do that for this patch, I was just mentioning it for the future).

Thanks for the hint. I'll try to remember that in the future.

This revision was automatically updated to reflect the committed changes.