Page MenuHomePhabricator

[clang] adds builtin `std::invoke` and `std::invoke_r`
AbandonedPublic

Authored by cjdb on Jul 31 2022, 10:06 PM.

Details

Reviewers
aaron.ballman
rsmith
NoQ
Group Reviewers
Restricted Project
Summary

std::invoke and std::invoke_r are fairly expensive in debug builds,
and are used extensively by standard ranges. By lifting these functions
into the compiler, we save a fair amount of space and improve run-time.

The changes in this commit were used to build a slightly modified
version of range-v3. range-v3 normally rolls its own invoke as a
function object, so the experiment deleted that code in favour of
using std::invoke.

Build timeBuild directory sizeCTest time
libstdc++ w/o D130867User: 465.04s478504 bytes13.19s
Sys: 58.98s(468 MB)
libstdc++ w/ D130867User: 468.49s456936 bytes7.79s
Sys: 57.31s(457 MB)
libc++ w/o D130867User: 627.67s795652 bytes11.88s
Sys: 67.16s(778 MB)
libc++ w/ D130867User: 626.03s781392 bytes8.32s
Sys: 65.77s(764MB)

Diff Detail

Event Timeline

cjdb created this revision.Jul 31 2022, 10:06 PM
Herald added a project: Restricted Project. · View Herald Transcript
cjdb requested review of this revision.Jul 31 2022, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2022, 10:06 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cjdb updated this revision to Diff 450581.Aug 6 2022, 4:59 PM
cjdb retitled this revision from WORK-IN-PROGRESS [clang] adds builtin `std::invoke` and `std::invoke_r` to [clang] adds builtin `std::invoke` and `std::invoke_r`.
cjdb edited the summary of this revision. (Show Details)

updates commit message

  • changes message so that it's accurate
  • no longer adding type traits in this commit
  • adds bechmark

updates diagnoses so that they're more accurate

cjdb updated this revision to Diff 450584.Aug 6 2022, 9:00 PM

adds logic to diagnose misqualified pointer-to-member functions

inclyc added a subscriber: inclyc.Aug 6 2022, 9:14 PM
cjdb updated this revision to Diff 450587.Aug 6 2022, 9:40 PM

rebases to ToT

cjdb added inline comments.Aug 7 2022, 5:39 PM
clang/lib/Sema/SemaExprCXX.cpp
5744–5749
cjdb updated this revision to Diff 452357.Aug 12 2022, 7:24 PM

applies a fix

cjdb updated this revision to Diff 452358.Aug 12 2022, 7:28 PM

rebase messed up diff

cjdb updated this revision to Diff 454908.Aug 23 2022, 11:29 AM

rebases onto main

aaron.ballman added a reviewer: Restricted Project.Tue, Aug 30, 11:42 AM

Thanks for working on this; these sort of improvements to compile time overhead are very much appreciated!

Has this been tested against libc++ (our preccommit CI doesn't test that), and if so, do all the results come back clean from their test suite? Did you try out any other large scale C++ projects to make sure they don't break?

clang/include/clang/Basic/DiagnosticSemaKinds.td
8411

Something's off here, what's a "wrapee"? ("to be a wrapee to a class compatible" doesn't seem grammatically correct.)

8413–8414

Can we reuse err_pointer_to_member_call_drops_quals instead of making a new diagnostic?

8415

Same question here, but about err_pointer_to_member_oper_value_classify instead.

8418–8430

Can we modify existing diagnostics for these as well?

It seems like in most cases, it's a matter of adding a select to use invoke terminology in all of these cases, which is why I'm asking.

clang/include/clang/Sema/Sema.h
4099

Since we're already touching the line anyway...

4101

Hmmm, I wonder if there's a way we can make this change without having to force every caller to think about std::invoke when they look at the signature for these calls? For example (and maybe this is a terrible idea, I'm thinking out loud), could we use an RAII object that specifies we're in a "building a call to invoke" context that is then checked within these functions (and we leave that RAII object before evaluating arguments to the invoke)?

I'm not strongly opposed to the current approach, but I'm worried that it adds complexity for an infrequent construct. I think we should try to keep the situations where we need information about a specific API as self-contained as possible because WG21 has shown more and more interest in the library being implemented in the compiler (so there's a scaling concern as well).

clang/lib/Sema/SemaExpr.cpp
267

Why do we want to suppress the diagnostic here?

14543

This also surprises me.

clang/lib/Sema/SemaExprCXX.cpp
5713–5715

Down with braces! Up with danger!

5736–5739

Yeah, it repeats a bunch of arguments to the calls, but at least it's not using function pointers and hoping the optimizer will undo them back into direct calls.

5759

Adding some standards citations about why you're handling things the way you are would be appreciated.

5776–5777

Do we need to look at the canonical type of FirstArgType? e.g., what if the user did something odd like wrapped reference_wrapper in a type alias and then used that?

clang/test/SemaCXX/builtin-std-invoke.cpp
295

It would be good to have a test which decays a use of std::invoke into a function pointer to make sure that's still possible.

Out of curiosity, would it be possible to do a benchmark to see how adding _LIBCPP_NODEBUG to std::invoke would compare to using builtins?

cjdb added a comment.Tue, Sep 6, 4:16 PM

Thanks for working on this; these sort of improvements to compile time overhead are very much appreciated!

Has this been tested against libc++ (our preccommit CI doesn't test that), and if so, do all the results come back clean from their test suite? Did you try out any other large scale C++ projects to make sure they don't break?

I've tested it with the libc++ tests (it caught an interesting thing or two), range-v3, and whatever else I've built since adding this patch to my local install of Clang. I can give a few more large projects a spin if you want higher confidence?

Out of curiosity, would it be possible to do a benchmark to see how adding _LIBCPP_NODEBUG to std::invoke would compare to using builtins?

They're within a few hundred bytes of each other when I also apply [[clang::always_inline]], but it's still worth applying the built-in. This patch helps multiple standard library implementations, so while I encourage libc++ to consider decorating std::invoke, I do think it's worth the implementation existing in Clang too. Another good reason is that by having it as a built-in lets me more easily add __is[_nothrow]_invocable and __invoke_result.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8411

"wrapee" refers to the type that's wrapped by std::reference_wrapper. I can't think of a better name for them, any suggestions?

8413–8414

Short answer: yes.

Long answer: I've found that "can't invoke X" to be really helpful when they've popped up, because it's very obvious that this is to do with std::invoke, so I'm partial to making err_pointer_to_member_call_drops_quals toggleable based on that. (I think you suggest this two comments down?)

clang/lib/Sema/SemaExpr.cpp
267

I think this is the one where I was getting the same diagnostic twice.

14543

Or maybe it was this one. One (or both) of the suppressions is because there was a duplicate error.

clang/test/SemaCXX/builtin-std-invoke.cpp
295

Is that allowed? I thought taking the address of std::invoke was ill-formed.

ldionne added a subscriber: ldionne.Thu, Sep 8, 1:17 PM

I think this is quite interesting, but it also raises the obvious question of: where do we stop? I think the following elements need to be taken into account:

  1. Implementing more stuff in the compiler adds complexity to the compiler, and does not decrease the complexity of the library because libc++ still needs to implement invoke on other compilers.
  2. It duplicates the tests. The test coverage added by this patch already exists in libc++ (or it should if we're missing it). This patch also seems to be only testing in C++17 mode, and only at compile-time. So technically, this has less coverage than libc++'s own tests for the same thing.
  3. Duplicating functionality in libc++ and in the compiler is confusing. For instance, it sets up the stage for people not knowing where a bug in std::invoke should be fixed. Is it in the compiler? In the library? In both? And the set of folks who can fix a bug in the library is not the same as the set of folks who can do it in the compiler.

Regarding the benefits of this patch, do I understand correctly that it had a roughly 1 second improvement in total User Time for libc++, and roughly a 3 seconds regression with libstdc++? If my math's right, that's a roughly 0.1% improvement on libc++ and a 0.6% pessimization on libstdc++, both of which I would argue are kind of in the noise. Also, ranges-v3 is not a typical code base -- it uses metaprogramming and invoke more than a traditional C++ code base, so the overall effect of this is likely to be less than that on a regular code base. Does this make sense, or am I mis interpreting the data?

So I think this is quite interesting, however I do question the benefits this provides in relation with its downsides. In particular, I am not sure that overriding non-trivial libc++ code like this is desirable. We're extremely happy with things like type traits (whose specification is super simple) being implemented in the compiler, but I'm not so sure about things like std::invoke, or even std::tuple. Instead, I'd personally be more interested in having builtins that we can use *from our implementation* to simplify various parts of the implementation (and make it faster). Thoughts?

cjdb abandoned this revision.Fri, Sep 9, 1:50 PM

I had not realised that libc++ was listed as a reviewer. I am going to abandon this revision, since I only wish to interact with that community when it is both business-critical and unavoidable.

I had not realised that libc++ was listed as a reviewer.

The change does have an impact on libc++, so I think it makes sense to look at it not only from the compiler perspective, but from the whole clang/libc++ ecosystem's perspective.

I am going to abandon this revision, since I only wish to interact with that community when it is both business-critical and unavoidable.

While you're not saying so explicitly, you seem to be implying some sort of fault on libc++ here, which I don't understand. I am trying to engage in a genuine and productive discussion. I'm asking questions to clarify my understanding of your benchmarks, raising IMO reasonable technical concerns with the patch, and doing so politely unless I missed something. I'm not saying we should not move forward with the patch -- there must be a reason why you put in all this effort into it. I'm merely raising concerns because no, this isn't a black-and-white situation -- there is a tradeoff with this patch.

I don't know whether your comment targets me personally or the libc++ community as a whole, but either way, honestly this sort of "I won't talk to you" behavior is kind of hurtful. If you feel there's something wrong with the libc++ community or with me, the LLVM Foundation has channels to remediate to that, and I encourage you (or anyone feeling bad about any part of LLVM) to use them.

cjdb added a comment.Sun, Sep 11, 8:16 PM

I had not realised that libc++ was listed as a reviewer.

The change does have an impact on libc++, so I think it makes sense to look at it not only from the compiler perspective, but from the whole clang/libc++ ecosystem's perspective.

I am going to abandon this revision, since I only wish to interact with that community when it is both business-critical and unavoidable.

While you're not saying so explicitly, you seem to be implying some sort of fault on libc++ here, which I don't understand. I am trying to engage in a genuine and productive discussion. I'm asking questions to clarify my understanding of your benchmarks, raising IMO reasonable technical concerns with the patch, and doing so politely unless I missed something. I'm not saying we should not move forward with the patch -- there must be a reason why you put in all this effort into it. I'm merely raising concerns because no, this isn't a black-and-white situation -- there is a tradeoff with this patch.

I don't know whether your comment targets me personally or the libc++ community as a whole, but either way, honestly this sort of "I won't talk to you" behavior is kind of hurtful. If you feel there's something wrong with the libc++ community or with me, the LLVM Foundation has channels to remediate to that, and I encourage you (or anyone feeling bad about any part of LLVM) to use them.

Please revise the emails I sent to you, titled 'libc++ involvement' on 2022-01-18 and 'RE: RE: libc++ involvement' on 2022-01-20, for the reasons why I do not wish to engage with the libc++ community. Friday's commentary simply documents why work on this patch has abruptly stopped.

I do not intend to discuss the matter any further in D130867.