[Clang] Default expression nesting limit should be higher

[philnik777]

C++17 gave us fold expressions, which are really nice compared to the templates we had to use before. Unfortunately, we currently can't use them in the places it makes the most sense (namely tuple), because they fail at only 256 types by default. It would be great if we could lift that restriction, so we can use fold expressions everywhere it makes sense.

CC @AaronBallman

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Nikolas Klauser (philnik777)

C++17 gave us fold expressions, which are really nice compared to the templates we had to use before. Unfortunately, we currently can't use them in the places it makes the most sense (namely `tuple`), because they fail at only 256 types by default. It would be great if we could lift that restriction, so we can use fold expressions everywhere it makes sense.

CC @AaronBallman

[dwblaikie]

Any sense of how high the limit would need to be/particular examples you're trying to support that aren't workable at the current limit?

[philnik777]

The maximum number of arguments we guarantee is 512, so 1024 seems like it should be more than enough. That's also the recommended number of template parameters an implementation should support (http://eel.is/c++draft/implimits#2.40).

[llvmbot]

@llvm/issue-subscribers-c-17

Author: Nikolas Klauser (philnik777)

C++17 gave us fold expressions, which are really nice compared to the templates we had to use before. Unfortunately, we currently can't use them in the places it makes the most sense (namely `tuple`), because they fail at only 256 types by default. It would be great if we could lift that restriction, so we can use fold expressions everywhere it makes sense.

CC @AaronBallman

[AaronBallman]

We do accept at least 1024 template parameters already: https://godbolt.org/z/qe6oddbzr -- this has to do with nested parentheses because a fold expression formally expands into nested paren expressions which is covered by http://eel.is/c++draft/implimits#2.4. Clang has an option -fbracket-depth for specifying a value beyond the implementation limit: https://godbolt.org/z/zjcMKjbje

That said, we could consider bumping the limit. GCC accepts 2048 by default but will eventually segfault if you go too high: https://godbolt.org/z/o96M7Tj46

[philnik777]

Sorry, I meant "we" as in "the libc++ tuple". I'm aware that there is a flag to increase the limit, but I'd rather avoid breaking people by limiting tuple to 256 elements without a flag now.

[AaronBallman]

From some limited early testing, I don't think we hit a huge performance problem by switching from 256 to 1024 as the default for bracket depths -- the performance impact seems to be roughly linear: https://godbolt.org/z/rMP4WYG8W (not that perf testing on godbolt is particularly scientific, lol)

256 parens: 655ms
512 parens: 681ms
1024 parens: 1024ms
2048 parens: 1959ms

[AaronBallman]

If someone wanted to work on this, they would update the default here:

llvm-project/clang/include/clang/Driver/Options.td

Line 7826 in 2117677

MarshallingInfoInt<LangOpts<"BracketDepth">, "256">;

and provide some more polished performance numbers regarding compile time overhead for whatever default is chosen.

[llvmbot]

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
3. Fix the issue locally.
4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
5. Create a Git commit.
6. Run git clang-format HEAD~1 to format your changes.
7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

Author: Nikolas Klauser (philnik777)

C++17 gave us fold expressions, which are really nice compared to the templates we had to use before. Unfortunately, we currently can't use them in the places it makes the most sense (namely `tuple`), because they fail at only 256 types by default. It would be great if we could lift that restriction, so we can use fold expressions everywhere it makes sense.

CC @AaronBallman

[philnik777]

@AaronBallman FYI you can also create a locale godbolt instance. It's still not super scientific, but much more comparable than the online version. Also, wouldn't the important performance be whether there is an impact with the same number of arguments? It's obviously going to slower the more work you give the compiler.

[AaronBallman]

@AaronBallman FYI you can also create a locale godbolt instance. It's still not super scientific, but much more comparable than the online version.

Good to know!

Also, wouldn't the important performance be whether there is an impact with the same number of arguments? It's obviously going to slower the more work you give the compiler.

Our template instantiation logic occasionally has surprising O(N^2) behaviors crop up, so I wanted to make sure the extra processing wasn't going to show a need for a significant refactoring (mostly to help me decide whether this was a good first issue or not).

[AaronBallman]

Reopening the issue because we had to revert the changes. This may no longer be a good first issue as we need to figure out why we're using so much stack space before we can usefully bump this limit.