[libcxx-commits] [PATCH] D131836: [libc++][CI] increases constexpr evaluation limit.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D131836#3746211 <https://reviews.llvm.org/D131836#3746211>, @ldionne wrote:

> I hate to rain on the parade, but I don't think this patch is desirable as-is. Indeed, we want to make sure that we compile our test suite with flags as similar as possible to what the users use. Otherwise, you end up not testing what matters, i.e. what users do.
> ...
> I just need to implement support for this new fancy form of `ADDITIONAL_COMPILE_FLAGS`. I'll look into it immediately, this would be useful on its own even regardless of this review.

As mention during our live review I disagree with the "rain on the parade". IMO this is how code review should work. With your suggestions and the new feature you added I think we have a better solution to solve this problem. Thanks!



================
Comment at: libcxx/test/std/utilities/charconv/charconv.to.chars/integral.pass.cpp:14
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-steps): -fconstexpr-steps=12712420
+// ADDITIONAL_COMPILE_FLAGS(has-fconstexpr-ops-limit): -fconstexpr-ops-limit=12712420
+
----------------
This isn't needed here (yet) but will be in the future. Add it here new to test whether the changes pass the CI.


================
Comment at: libcxx/utils/libcxx/test/features.py:43
+  # The default maximum number of operations in a constexpr function are too
+  # small for some tests. A "correct" value has been determined empirical.
+  Feature(name='clang-constexpr-steps',
----------------
var-const wrote:
> Mordante wrote:
> > var-const wrote:
> > > var-const wrote:
> > > > Ultranit: `s/empirical/empirically/`.
> > > I think it would be helpful to write the default value for comparison (I think it's `1048576` for Clang -- see `clang/include/clang/Basic/LangOptions.def`).
> > I don' think that's really helpful, Clang may change this at some point and then the comment is out of date. When really wanted to have the number I'd rather link to the source of truth.
> I don't feel very strongly, but I'd still add it. I think it's obvious to the reader that this number may be out of date, but it could be made explicit by adding a note like "at the time of writing". What I find useful here isn't so much the exact number as having a point of comparison -- i.e., are we increasing the default limit two times, ten times, a hundred times, etc.?
With the suggested approach of @ldionne the hard-coded value is removed here.


================
Comment at: libcxx/utils/libcxx/test/features.py:46
+          when=lambda cfg: hasCompileFlag(cfg, '-fconstexpr-steps=1'),
+          actions=[AddCompileFlag('-fconstexpr-steps=128000000')]),
+
----------------
var-const wrote:
> philnik wrote:
> > Mordante wrote:
> > > philnik wrote:
> > > > Are we sure we want to bring this right to the maximum? This could increase the built time by 100x when you actually have bad code.
> > > I can remove one 0 and it still works, but I like to have a bit of a margin. Lowering it further fails in the test.
> > I'd be a lot more comfortable with "just" a 10x increase. We can still bump it higher if we really need to later. By that time we might want to consider adding some easy way to disable this for specific tests to not have the compiler working for ages just to let us know we have a bug in our code. The `ranges.transform.pass.cpp` test is already horrible to work in. I can't imagine how it would be if we needed this step limit.
> I think ideally we'd keep the default limit and then increase it as an opt-in for certain tests (that way, the increased number can be finer-grained to the needs of each test file). But currently I think there is no way to pass flags to e.g. only Clang.
With the suggested approach of @ldionne the limit will be per test and no longer a global value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131836/new/

https://reviews.llvm.org/D131836