Too aggresive C++23 static call operator optimization #67976

shtanko-sv · 2023-10-02T11:36:14Z

Hello,

It looks like clang is too aggressive for C++23 static call optimization. Here is the simplified example of real code:

#include <functional>
#include <iostream>

template <typename Tag>
struct Wrapper {
    template <typename F, typename... Args>
    static void operator()(F&& callable, Args&&... args) {
        Invoke(std::forward<F>(callable), std::forward<Args>(args)...);
    }

    template <typename F, typename... Args>
    static void Invoke(F&& callable, Args&&... args) {
        std::invoke(std::forward<F>(callable), std::forward<Args>(args)...);
    }
};

struct Manager {
    template <typename SingletonClass>
    static SingletonClass& Get() {
        std::cout << "get call\n";
        static SingletonClass instance;
        return instance;
    }
};

struct Tag {};

int main()
{
    using TaggedWrapper = Wrapper<Tag>;
    Manager::Get<TaggedWrapper>()([] { std::cout << "Function\n"; });
}

We assume that Manager::Get() will be called and get call will be printed, but it doesn't happen. If change call in main to Manager::Get<TaggedWrapper>().Invoke([] { std::cout << "Function\n"; }); it works as expected. get cal also will be printed if remove static from operator(). It reproducible even with -O0. clang-17.0.1

The text was updated successfully, but these errors were encountered:

dtcxzyw · 2023-10-02T11:45:48Z

Godbolt: https://godbolt.org/z/d8sG1so1h
It seems like clang didn't emit llvm ir for the statement std::cout << "get call\n";.

llvmbot · 2023-10-02T11:46:46Z

@llvm/issue-subscribers-bug

Hello,

It looks like clang is too aggressive for C++23 static call optimization. Here is the simplified example of real code:

#include &lt;functional&gt;
#include &lt;iostream&gt;

template &lt;typename Tag&gt;
struct Wrapper {
    template &lt;typename F, typename... Args&gt;
    static void operator()(F&amp;&amp; callable, Args&amp;&amp;... args) {
        Invoke(std::forward&lt;F&gt;(callable), std::forward&lt;Args&gt;(args)...);
    }

    template &lt;typename F, typename... Args&gt;
    static void Invoke(F&amp;&amp; callable, Args&amp;&amp;... args) {
        std::invoke(std::forward&lt;F&gt;(callable), std::forward&lt;Args&gt;(args)...);
    }
};

struct Manager {
    template &lt;typename SingletonClass&gt;
    static SingletonClass&amp; Get() {
        std::cout &lt;&lt; "get call\n";
        static SingletonClass instance;
        return instance;
    }
};

struct Tag {};

int main()
{
    using TaggedWrapper = Wrapper&lt;Tag&gt;;
    Manager::Get&lt;TaggedWrapper&gt;()([] { std::cout &lt;&lt; "Function\n"; });
}

We assume that Manager::Get() will be called and get call will be printed, but it doesn't happen. If change call in main to Manager::Get<TaggedWrapper>().Invoke([] { std::cout << "Function\n"; }); it works as expected. get cal also will be printed if remove static from operator(). It reproducible even with -O0. clang-17.0.1

dtcxzyw · 2023-10-02T12:05:37Z

Reduced code: https://godbolt.org/z/q84zajYGs

#include <iostream>

struct Wrapper {
    static void operator()() {
        std::cout << "Function\n";
    }
};

static Wrapper instance;
struct Manager {
    static Wrapper& Get() {
        std::cout << "get call\n";
        return instance;
    }
};

int main() {
    Manager::Get()();
    return 0;
}

The expression Manager::Get() is not evaluated.
Related patch: https://reviews.llvm.org/D133659

llvmbot · 2023-10-02T12:57:33Z

@llvm/issue-subscribers-clang-frontend

Hello,

It looks like clang is too aggressive for C++23 static call optimization. Here is the simplified example of real code:

#include &lt;functional&gt;
#include &lt;iostream&gt;

template &lt;typename Tag&gt;
struct Wrapper {
    template &lt;typename F, typename... Args&gt;
    static void operator()(F&amp;&amp; callable, Args&amp;&amp;... args) {
        Invoke(std::forward&lt;F&gt;(callable), std::forward&lt;Args&gt;(args)...);
    }

    template &lt;typename F, typename... Args&gt;
    static void Invoke(F&amp;&amp; callable, Args&amp;&amp;... args) {
        std::invoke(std::forward&lt;F&gt;(callable), std::forward&lt;Args&gt;(args)...);
    }
};

struct Manager {
    template &lt;typename SingletonClass&gt;
    static SingletonClass&amp; Get() {
        std::cout &lt;&lt; "get call\n";
        static SingletonClass instance;
        return instance;
    }
};

struct Tag {};

int main()
{
    using TaggedWrapper = Wrapper&lt;Tag&gt;;
    Manager::Get&lt;TaggedWrapper&gt;()([] { std::cout &lt;&lt; "Function\n"; });
}

We assume that Manager::Get() will be called and get call will be printed, but it doesn't happen. If change call in main to Manager::Get<TaggedWrapper>().Invoke([] { std::cout << "Function\n"; }); it works as expected. get cal also will be printed if remove static from operator(). It reproducible even with -O0. clang-17.0.1

llvmbot · 2023-10-02T13:07:42Z

@llvm/issue-subscribers-clang-codegen

Hello,

It looks like clang is too aggressive for C++23 static call optimization. Here is the simplified example of real code:

#include &lt;functional&gt;
#include &lt;iostream&gt;

template &lt;typename Tag&gt;
struct Wrapper {
    template &lt;typename F, typename... Args&gt;
    static void operator()(F&amp;&amp; callable, Args&amp;&amp;... args) {
        Invoke(std::forward&lt;F&gt;(callable), std::forward&lt;Args&gt;(args)...);
    }

    template &lt;typename F, typename... Args&gt;
    static void Invoke(F&amp;&amp; callable, Args&amp;&amp;... args) {
        std::invoke(std::forward&lt;F&gt;(callable), std::forward&lt;Args&gt;(args)...);
    }
};

struct Manager {
    template &lt;typename SingletonClass&gt;
    static SingletonClass&amp; Get() {
        std::cout &lt;&lt; "get call\n";
        static SingletonClass instance;
        return instance;
    }
};

struct Tag {};

int main()
{
    using TaggedWrapper = Wrapper&lt;Tag&gt;;
    Manager::Get&lt;TaggedWrapper&gt;()([] { std::cout &lt;&lt; "Function\n"; });
}

We assume that Manager::Get() will be called and get call will be printed, but it doesn't happen. If change call in main to Manager::Get<TaggedWrapper>().Invoke([] { std::cout << "Function\n"; }); it works as expected. get cal also will be printed if remove static from operator(). It reproducible even with -O0. clang-17.0.1

shafik · 2023-10-02T20:34:27Z

If we look at the AST: https://godbolt.org/z/7xExo9oob

`-FunctionDecl <line:17:1, line:20:1> line:17:5 main 'int ()'
  `-CompoundStmt <col:12, line:20:1>
    |-CallExpr <line:18:19, col:20> 'void'
    | `-ImplicitCastExpr <col:19, col:20> 'void (*)()' <FunctionToPointerDecay>
    |   `-DeclRefExpr <col:19, col:20> 'void ()' lvalue CXXMethod 0xf6ea300 'operator()' 'void ()'

It is already elided away. CC @zygoloid

zygoloid · 2023-10-02T23:04:34Z

Yeah, that AST is clearly wrong. The callee of the outer call expression should be a member access, like it is for a call to operator() by name:

`-FunctionDecl <line:17:1, line:20:1> line:17:5 main 'int ()'
  `-CompoundStmt <col:12, line:20:1>
    |-CallExpr <line:18:5, col:31> 'void'
    | `-ImplicitCastExpr <col:5, col:29> 'void (*)()' <FunctionToPointerDecay>
    |   `-MemberExpr <col:5, col:29> 'void ()' lvalue .operator() 0xf744560
    |     `-CallExpr <col:5, col:18> 'Wrapper':'Wrapper' lvalue
    |       `-ImplicitCastExpr <col:5, col:14> 'Wrapper &(*)()' <FunctionToPointerDecay>
    |         `-DeclRefExpr <col:5, col:14> 'Wrapper &()' lvalue CXXMethod 0xf746478 'Get' 'Wrapper &()'
    |           `-NestedNameSpecifier TypeSpec 'Manager'

The outer call expression should be a CXXOperatorCallExpr, to represent the fact that we're implementing operator syntax via a call to operator$ (in this case, $ = ()).

SuperSodaSea · 2023-10-03T03:55:34Z

Compare the AST of (static) member/operator calls (https://godbolt.org/z/rn3s49GEz):

struct Foo {
    void f() {}
    void operator()() {}
};
struct Bar {
    static void f() {}
    static void operator()() {}
};

template <typename T>
T& g() {
    static T t;
    return t;
}

g<Foo>().f(); // Non-static member call
/*
CXXMemberCallExpr <line:17:5, col:16> 'void'
`-MemberExpr <col:5, col:14> '<bound member function type>' .f 0xbae3a90
 `-CallExpr <col:5, col:12> 'Foo':'Foo' lvalue
  `-ImplicitCastExpr <col:5, col:10> 'Foo &(*)()' <FunctionToPointerDecay>
   `-DeclRefExpr <col:5, col:10> 'Foo &()' lvalue Function 0xbb03710 'g' 'Foo &()' (FunctionTemplate 0xbae42f0 'g')
*/

g<Foo>()(); // Non-static operator call
/*
CXXOperatorCallExpr <line:18:5, col:14> 'void' '()'
|-ImplicitCastExpr <col:13, col:14> 'void (*)()' <FunctionToPointerDecay>
| `-DeclRefExpr <col:13, col:14> 'void ()' lvalue CXXMethod 0xbae3b78 'operator()' 'void ()'
`-CallExpr <col:5, col:12> 'Foo':'Foo' lvalue
  `-ImplicitCastExpr <col:5, col:10> 'Foo &(*)()' <FunctionToPointerDecay>
    `-DeclRefExpr <col:5, col:10> 'Foo &()' lvalue Function 0xbb03710 'g' 'Foo &()' (FunctionTemplate 0xbae42f0 'g')
*/

g<Bar>().f(); // Static member call (with object)
/*
CallExpr <line:19:5, col:16> 'void'
`-ImplicitCastExpr <col:5, col:14> 'void (*)()' <FunctionToPointerDecay>
  `-MemberExpr <col:5, col:14> 'void ()' lvalue .f 0xbae3eb0
    `-CallExpr <col:5, col:12> 'Bar':'Bar' lvalue
      `-ImplicitCastExpr <col:5, col:10> 'Bar &(*)()' <FunctionToPointerDecay>
        `-DeclRefExpr <col:5, col:10> 'Bar &()' lvalue Function 0xbb03f80 'g' 'Bar &()' (FunctionTemplate 0xbae42f0 'g')
*/

g<Bar>()(); // Static operator call
/*
CallExpr <line:20:13, col:14> 'void'
`-ImplicitCastExpr <col:13, col:14> 'void (*)()' <FunctionToPointerDecay>
  `-DeclRefExpr <col:13, col:14> 'void ()' lvalue CXXMethod 0xbae3f98 'operator()' 'void ()'
*/

If we use CXXOperatorCallExpr for static operator calls, should we use CXXMemberCallExpr for static member calls (with object) for consistency? (Although the current AST for static member calls can produce the correct code)

zygoloid · 2023-10-03T06:12:04Z

If we use CXXOperatorCallExpr for static operator calls, should we use CXXMemberCallExpr for static member calls (with object) for consistency? (Although the current AST for static member calls can produce the correct code)

Hm, good question, but I don't think so. The only real purpose of CXXMemberCallExpr, as distinguished from CallExpr, is to provide supporting functionality for handling an object parameter. I think it's fine to model these two the same:

struct A {
  static void f();
  void (&g)();
} a;
// A `CallExpr` whose callee is a `MemberExpr`.
a.f();
// A `CallExpr` whose callee is a `MemberExpr`.
a.g();

In contrast, we use CXXOperatorCallExpr for every other case where an operator $ (including function call) is rewritten to a call to a operator$ function, so it would be a departure to use anything else here. Also, CXXOperatorCallExpr already handles both calls to non-static member functions and to non-member functions, so code dealing with it is already in a good place to handle calls to static member functions (which ought to behave like non-member functions in most ways).

SuperSodaSea · 2023-10-04T15:26:30Z

@dtcxzyw asked me if I'd like to work on this issue, so I'm trying to write a patch for it now. Will create a PR in a few days.

SuperSodaSea · 2023-10-07T13:51:17Z

PR made: #68485

### Description clang don't evaluate the object argument of `static operator()` and `static operator[]` currently, for example: ```cpp #include <iostream> struct Foo { static int operator()(int x, int y) { std::cout << "Foo::operator()" << std::endl; return x + y; } static int operator[](int x, int y) { std::cout << "Foo::operator[]" << std::endl; return x + y; } }; Foo getFoo() { std::cout << "getFoo()" << std::endl; return {}; } int main() { std::cout << getFoo()(1, 2) << std::endl; std::cout << getFoo()[1, 2] << std::endl; } ``` `getFoo()` is expected to be called, but clang don't call it currently (17.0.2). This PR fixes this issue. Fixes #67976. ### Walkthrough - **clang/lib/Sema/SemaOverload.cpp** - **`Sema::CreateOverloadedArraySubscriptExpr` & `Sema::BuildCallToObjectOfClassType`** Previously clang generate `CallExpr` for static operators, ignoring the object argument. In this PR `CXXOperatorCallExpr` is generated for static operators instead, with the object argument as the first argument. - **`TryObjectArgumentInitialization`** `const` / `volatile` objects are allowed for static methods, so that we can call static operators on them. - **clang/lib/CodeGen/CGExpr.cpp** - **`CodeGenFunction::EmitCall`** CodeGen changes for `CXXOperatorCallExpr` with static operators: emit and ignore the object argument first, then emit the operator call. - **clang/lib/AST/ExprConstant.cpp** - **`‎ExprEvaluatorBase::handleCallExpr‎`** Evaluation of static operators in constexpr also need some small changes to work, so that the arguments won't be out of position. - **clang/lib/Sema/SemaChecking.cpp** - **`Sema::CheckFunctionCall`** Code for argument checking also need to be modify, or it will fail the test `clang/test/SemaCXX/overloaded-operator-decl.cpp`. ### Tests - **Added:** - **clang/test/AST/ast-dump-static-operators.cpp** Verify the AST generated for static operators. - **clang/test/SemaCXX/cxx2b-static-operator.cpp** Static operators should be able to be called on const / volatile objects. - **Modified:** - **clang/test/CodeGenCXX/cxx2b-static-call-operator.cpp** - **clang/test/CodeGenCXX/cxx2b-static-subscript-operator.cpp** Matching the new CodeGen. ### Documentation - **clang/docs/ReleaseNotes.rst** Update release notes. --------- Co-authored-by: Shafik Yaghmour <[email protected]> Co-authored-by: cor3ntin <[email protected]> Co-authored-by: Aaron Ballman <[email protected]>

…0108) This re-applies 30155fc with a fix for clangd. ### Description clang don't evaluate the object argument of `static operator()` and `static operator[]` currently, for example: ```cpp #include <iostream> struct Foo { static int operator()(int x, int y) { std::cout << "Foo::operator()" << std::endl; return x + y; } static int operator[](int x, int y) { std::cout << "Foo::operator[]" << std::endl; return x + y; } }; Foo getFoo() { std::cout << "getFoo()" << std::endl; return {}; } int main() { std::cout << getFoo()(1, 2) << std::endl; std::cout << getFoo()[1, 2] << std::endl; } ``` `getFoo()` is expected to be called, but clang don't call it currently (17.0.6). This PR fixes this issue. Fixes #67976, reland #68485. ### Walkthrough - **clang/lib/Sema/SemaOverload.cpp** - **`Sema::CreateOverloadedArraySubscriptExpr` & `Sema::BuildCallToObjectOfClassType`** Previously clang generate `CallExpr` for static operators, ignoring the object argument. In this PR `CXXOperatorCallExpr` is generated for static operators instead, with the object argument as the first argument. - **`TryObjectArgumentInitialization`** `const` / `volatile` objects are allowed for static methods, so that we can call static operators on them. - **clang/lib/CodeGen/CGExpr.cpp** - **`CodeGenFunction::EmitCall`** CodeGen changes for `CXXOperatorCallExpr` with static operators: emit and ignore the object argument first, then emit the operator call. - **clang/lib/AST/ExprConstant.cpp** - **`‎ExprEvaluatorBase::handleCallExpr‎`** Evaluation of static operators in constexpr also need some small changes to work, so that the arguments won't be out of position. - **clang/lib/Sema/SemaChecking.cpp** - **`Sema::CheckFunctionCall`** Code for argument checking also need to be modify, or it will fail the test `clang/test/SemaCXX/overloaded-operator-decl.cpp`. - **clang-tools-extra/clangd/InlayHints.cpp** - **`InlayHintVisitor::VisitCallExpr`** Now that the `CXXOperatorCallExpr` for static operators also have object argument, we should also take care of this situation in clangd. ### Tests - **Added:** - **clang/test/AST/ast-dump-static-operators.cpp** Verify the AST generated for static operators. - **clang/test/SemaCXX/cxx2b-static-operator.cpp** Static operators should be able to be called on const / volatile objects. - **Modified:** - **clang/test/CodeGenCXX/cxx2b-static-call-operator.cpp** - **clang/test/CodeGenCXX/cxx2b-static-subscript-operator.cpp** Matching the new CodeGen. ### Documentation - **clang/docs/ReleaseNotes.rst** Update release notes. --------- Co-authored-by: Shafik Yaghmour <[email protected]> Co-authored-by: cor3ntin <[email protected]> Co-authored-by: Aaron Ballman <[email protected]>

…eland)' to release/18.x (cherry picked from commit ee01a2c) This re-applies 30155fc with a fix for clangd. clang don't evaluate the object argument of `static operator()` and `static operator[]` currently, for example: ```cpp struct Foo { static int operator()(int x, int y) { std::cout << "Foo::operator()" << std::endl; return x + y; } static int operator[](int x, int y) { std::cout << "Foo::operator[]" << std::endl; return x + y; } }; Foo getFoo() { std::cout << "getFoo()" << std::endl; return {}; } int main() { std::cout << getFoo()(1, 2) << std::endl; std::cout << getFoo()[1, 2] << std::endl; } ``` `getFoo()` is expected to be called, but clang don't call it currently (17.0.6). This PR fixes this issue. Fixes llvm#67976, reland llvm#68485. - **clang/lib/Sema/SemaOverload.cpp** - **`Sema::CreateOverloadedArraySubscriptExpr` & `Sema::BuildCallToObjectOfClassType`** Previously clang generate `CallExpr` for static operators, ignoring the object argument. In this PR `CXXOperatorCallExpr` is generated for static operators instead, with the object argument as the first argument. - **`TryObjectArgumentInitialization`** `const` / `volatile` objects are allowed for static methods, so that we can call static operators on them. - **clang/lib/CodeGen/CGExpr.cpp** - **`CodeGenFunction::EmitCall`** CodeGen changes for `CXXOperatorCallExpr` with static operators: emit and ignore the object argument first, then emit the operator call. - **clang/lib/AST/ExprConstant.cpp** - **`‎ExprEvaluatorBase::handleCallExpr‎`** Evaluation of static operators in constexpr also need some small changes to work, so that the arguments won't be out of position. - **clang/lib/Sema/SemaChecking.cpp** - **`Sema::CheckFunctionCall`** Code for argument checking also need to be modify, or it will fail the test `clang/test/SemaCXX/overloaded-operator-decl.cpp`. - **clang-tools-extra/clangd/InlayHints.cpp** - **`InlayHintVisitor::VisitCallExpr`** Now that the `CXXOperatorCallExpr` for static operators also have object argument, we should also take care of this situation in clangd. - **Added:** - **clang/test/AST/ast-dump-static-operators.cpp** Verify the AST generated for static operators. - **clang/test/SemaCXX/cxx2b-static-operator.cpp** Static operators should be able to be called on const / volatile objects. - **Modified:** - **clang/test/CodeGenCXX/cxx2b-static-call-operator.cpp** - **clang/test/CodeGenCXX/cxx2b-static-subscript-operator.cpp** Matching the new CodeGen. - **clang/docs/ReleaseNotes.rst** Update release notes. --------- Co-authored-by: Shafik Yaghmour <[email protected]> Co-authored-by: cor3ntin <[email protected]> Co-authored-by: Aaron Ballman <[email protected]>

SuperSodaSea · 2024-02-01T13:15:18Z

https://godbolt.org/z/qeoEeaWxh
Fix confirmed on trunk! 🎉

Trying to catch up with the 18.x release now -> #80109

github-actions bot added the new issue label Oct 2, 2023

dtcxzyw added clang Clang issues not falling into any other category and removed new issue labels Oct 2, 2023

dtcxzyw added the bug Indicates an unexpected problem or unintended behavior label Oct 2, 2023

dtcxzyw self-assigned this Oct 2, 2023

dtcxzyw added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed clang Clang issues not falling into any other category labels Oct 2, 2023

cor3ntin added the clang:codegen label Oct 2, 2023

dtcxzyw added the c++23 label Oct 2, 2023

dtcxzyw assigned SuperSodaSea and unassigned dtcxzyw Oct 4, 2023

SuperSodaSea mentioned this issue Oct 7, 2023

[clang] static operators should evaluate object argument #68485

Merged

shafik mentioned this issue Jan 17, 2024

[Clang] E[...]/E(...) does not evaluate E if it calls a static overload #78355

Closed

AaronBallman closed this as completed in #68485 Jan 30, 2024

SuperSodaSea mentioned this issue Jan 31, 2024

[clang] static operators should evaluate object argument (reland) #80108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Too aggresive C++23 static call operator optimization #67976

Too aggresive C++23 static call operator optimization #67976

shtanko-sv commented Oct 2, 2023 •

edited

Loading

dtcxzyw commented Oct 2, 2023

llvmbot commented Oct 2, 2023

dtcxzyw commented Oct 2, 2023 •

edited

Loading

llvmbot commented Oct 2, 2023

llvmbot commented Oct 2, 2023

shafik commented Oct 2, 2023

zygoloid commented Oct 2, 2023

SuperSodaSea commented Oct 3, 2023 •

edited

Loading

zygoloid commented Oct 3, 2023

SuperSodaSea commented Oct 4, 2023

SuperSodaSea commented Oct 7, 2023

SuperSodaSea commented Feb 1, 2024

Too aggresive C++23 static call operator optimization #67976

Too aggresive C++23 static call operator optimization #67976

Comments

shtanko-sv commented Oct 2, 2023 • edited Loading

dtcxzyw commented Oct 2, 2023

llvmbot commented Oct 2, 2023

dtcxzyw commented Oct 2, 2023 • edited Loading

llvmbot commented Oct 2, 2023

llvmbot commented Oct 2, 2023

shafik commented Oct 2, 2023

zygoloid commented Oct 2, 2023

SuperSodaSea commented Oct 3, 2023 • edited Loading

zygoloid commented Oct 3, 2023

SuperSodaSea commented Oct 4, 2023

SuperSodaSea commented Oct 7, 2023

SuperSodaSea commented Feb 1, 2024

shtanko-sv commented Oct 2, 2023 •

edited

Loading

dtcxzyw commented Oct 2, 2023 •

edited

Loading

SuperSodaSea commented Oct 3, 2023 •

edited

Loading