This is an archive of the discontinued LLVM Phabricator instance.

[Support] move llvm::llvm_is_multithread to header, NFC
ClosedPublic

Authored by junaire on Aug 6 2022, 1:13 AM.

Diff Detail

Event Timeline

junaire created this revision.Aug 6 2022, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 1:13 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
junaire requested review of this revision.Aug 6 2022, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 1:13 AM
junaire updated this revision to Diff 450502.Aug 6 2022, 2:14 AM

Fix the build

junaire updated this revision to Diff 450504.Aug 6 2022, 2:56 AM

Clang-format

nikic added a comment.Aug 6 2022, 7:11 AM

I don't think we should use if constexpr if we don't actually require it's semantics. A normal if will get optimized just fine. The only actual change here is that the llvm_is_multithreaded() definition gets moved into the header, which allows this to optimize without LTO.

barannikov88 added inline comments.
llvm/include/llvm/Support/Mutex.h
48–49

Wrong indentation here and in lock.

llvm/include/llvm/Support/Threading.h
54–56
barannikov88 added inline comments.Aug 6 2022, 9:40 AM
llvm/include/llvm/Support/Mutex.h
28

There is also SmartRWMutex in RWMutex.h.

thakis added a comment.Aug 7 2022, 7:26 AM

I don't think we should use if constexpr if we don't actually require it's semantics. A normal if will get optimized just fine. The only actual change here is that the llvm_is_multithreaded() definition gets moved into the header, which allows this to optimize without LTO.

I agree with this, FWIW.

junaire updated this revision to Diff 450623.Aug 7 2022, 7:33 AM

I'm not if this patch is worth to continue...

junaire retitled this revision from [Support] Use constexpr if to [Support] move llvm::llvm_is_multithread to header, NFC.Aug 7 2022, 7:35 AM
junaire edited the summary of this revision. (Show Details)
thakis added a comment.Aug 7 2022, 7:52 AM

Removing the else-after-ifs seems fine to me.

llvm/include/llvm/Support/Mutex.h
36–37

run git clang-format main

junaire added inline comments.Aug 7 2022, 7:58 AM
llvm/include/llvm/Support/Mutex.h
36–37

run git clang-format main

I have already formatted the patch via git clang-format HEAD~1. What's wrong with it?
clang-format version: Ubuntu clang-format version 14.0.0-1ubuntu1

junaire updated this revision to Diff 450628.Aug 7 2022, 8:04 AM

git clang-format HEAD~1 can't format code that not been touched, format by hand.

barannikov88 added inline comments.Aug 7 2022, 8:05 AM
llvm/include/llvm/Support/Mutex.h
48–49

Still holds

barannikov88 added inline comments.Aug 7 2022, 8:07 AM
llvm/include/llvm/Support/Mutex.h
48–49

Never mind, compared wrong revisions

barannikov88 accepted this revision.Aug 7 2022, 8:24 AM
This revision is now accepted and ready to land.Aug 7 2022, 8:24 AM
This revision was landed with ongoing or failed builds.Aug 7 2022, 5:50 PM
This revision was automatically updated to reflect the committed changes.