Do you need help landing the change?

Looks good with one suggestion.

I wonder what motivated the patch. Is this a performance optimization? If so, do you have any measurements?

In D113578#3195448, @fhahn wrote:

In D113578#3195263, @alexfh wrote:

Scratch that. It wasn't a valid experiment at all. Looking further.

Ok, please let me know if there's a regression. Otherwise I'll go ahead with the fix, as the compile-time-tracker shows this to be compile-time neutral (or a tiny bit positive).

In D113578#3195175, @alexfh wrote:

In D113578#3195059, @fhahn wrote:

In D113578#3195050, @alexfh wrote:

In D113578#3194811, @fhahn wrote:

I took a closer look and confirmed that the patch indeed slightly increases the call stack for the reproducer and this causes the stack overflow.

The issue is roughly the following: we apply the loop guards to the last loop in a large chain of loops. The loop guard itself is the exit condition of an earlier loop. To construct the SCEV, we compute the exit count along one of the code paths. This in turn applies loop guards which is in an earlier predecessor loop. To construct the SCEV, we again need to compute the exit count of this loop, applying loop guards and so on.

This can be fixed by adjusting the order we apply loop guards. At the moment, the guard closest to the starting loop is evaluated first, triggering the large evaluation chain. If we instead apply the earliest guard first no excessive call chain is needed. This seems to have a tiny positive compile-time impact http://llvm-compile-time-tracker.com/compare.php?from=529833377ccdf4381f8bc9961bfa96ec4f5e2eed&to=b07452a70ac22ae4b1f0dbf4b84df3ee44c171a1&stat=instructions

It is not quite NFC, because the order in which guards are applied can impact the result in some cases. I looked into some and so far it looks like this new order makes applying loop guards slightly more effective. I am planning on isolating a test that shows the improved analysis results and commit the fix soonish.

Thanks! I guess, I can come up a couple of distinct test cases that have the same effect (compiler stack overflow). Would you like to have a look at them as well?

The fix is available here: https://github.com/llvm/llvm-project/commit/b07452a70ac22ae4b1f0dbf4b84df3ee44c171a1, in case you want to give it a try against the other test cases.

The patch fixes the crash, but it looks like the compilation time with the patch grows by a factor of ~7 for one of the problematic files. I'll try to make a cleaner experiment with two compilers built with exactly the same compiler options, but I doubt it will change much.

In D113578#3195059, @fhahn wrote:

In D113578#3195050, @alexfh wrote:

In D113578#3194811, @fhahn wrote:

I took a closer look and confirmed that the patch indeed slightly increases the call stack for the reproducer and this causes the stack overflow.

The issue is roughly the following: we apply the loop guards to the last loop in a large chain of loops. The loop guard itself is the exit condition of an earlier loop. To construct the SCEV, we compute the exit count along one of the code paths. This in turn applies loop guards which is in an earlier predecessor loop. To construct the SCEV, we again need to compute the exit count of this loop, applying loop guards and so on.

This can be fixed by adjusting the order we apply loop guards. At the moment, the guard closest to the starting loop is evaluated first, triggering the large evaluation chain. If we instead apply the earliest guard first no excessive call chain is needed. This seems to have a tiny positive compile-time impact http://llvm-compile-time-tracker.com/compare.php?from=529833377ccdf4381f8bc9961bfa96ec4f5e2eed&to=b07452a70ac22ae4b1f0dbf4b84df3ee44c171a1&stat=instructions

It is not quite NFC, because the order in which guards are applied can impact the result in some cases. I looked into some and so far it looks like this new order makes applying loop guards slightly more effective. I am planning on isolating a test that shows the improved analysis results and commit the fix soonish.

Thanks! I guess, I can come up a couple of distinct test cases that have the same effect (compiler stack overflow). Would you like to have a look at them as well?

The fix is available here: https://github.com/llvm/llvm-project/commit/b07452a70ac22ae4b1f0dbf4b84df3ee44c171a1, in case you want to give it a try against the other test cases.

(general discussion, not necessarily related to this patch)

In D113578#3194811, @fhahn wrote:

I took a closer look and confirmed that the patch indeed slightly increases the call stack for the reproducer and this causes the stack overflow.

The issue is roughly the following: we apply the loop guards to the last loop in a large chain of loops. The loop guard itself is the exit condition of an earlier loop. To construct the SCEV, we compute the exit count along one of the code paths. This in turn applies loop guards which is in an earlier predecessor loop. To construct the SCEV, we again need to compute the exit count of this loop, applying loop guards and so on.

This can be fixed by adjusting the order we apply loop guards. At the moment, the guard closest to the starting loop is evaluated first, triggering the large evaluation chain. If we instead apply the earliest guard first no excessive call chain is needed. This seems to have a tiny positive compile-time impact http://llvm-compile-time-tracker.com/compare.php?from=529833377ccdf4381f8bc9961bfa96ec4f5e2eed&to=b07452a70ac22ae4b1f0dbf4b84df3ee44c171a1&stat=instructions

It is not quite NFC, because the order in which guards are applied can impact the result in some cases. I looked into some and so far it looks like this new order makes applying loop guards slightly more effective. I am planning on isolating a test that shows the improved analysis results and commit the fix soonish.

In D113578#3192532, @reames wrote:

In D113578#3188065, @alexfh wrote:

Unfortunately, the burden of mitigating the underlying issue in practice frequently lies with the author of the patch exposing the issue (unless someone else volunteers to do this).

This isn't quite as clear cut as you make it out to be here. Yes, we will frequently fix issues exposed in the process of introducing an unrelated bug. However, the standard is not (and has never been) any reported issue which happens to be exposed. We will certainly fix upstream tests, common workloads, and quickly reported issues, but at some point, the responsibility shifts to the downstream maintainer. That's one of the reasons rapid reporting is so strongly encouraged.

To be clear, I'm not commenting on which this situation might be. I'm just making the general point that this is a lot more complicated than your wording implies.

In D113578#3191883, @fhahn wrote:

Thanks for the heads-up, I'll take a look now. But from the stack trace you shared, I doubt that the patch itself is causing the crash, but rather exposes an existing bug in GetMinTrailingZeros/computeKnownBits

A bit cleaner test case:

I observe the problem on Linux on x86-64.

I've created a standalone test case for the issue:

Hi Florian,

LG per https://reviews.llvm.org/D110867#3185369

Reduced the problematic file to this:

FYI, the last re-commit of this patch introduced another compile time regression. See https://reviews.llvm.org/rGc93f93b2e3f28997f794265089fb8138dd5b5f13 for more information.

In rGc93f93b2e3f28997f794265089fb8138dd5b5f13#1039669, @bgraur wrote:

Early heads up that this revision causes a large regression in compilation time for some of our internal source files: we are seeing an almost 20x increase in compilation times for some files (from 42s to 728s).

I'm working on a reproducer which I'm going to upload when ready.

In D113148#3127646, @ymandel wrote:

This looks really good. An impressive effort! Just a few changes. Also, please ping Alex to get his opinion on the high level issue.

In D113027#3132727, @ldionne wrote:

In D113027#3131369, @alexfh wrote:

In D113027#3125511, @mstorsjo wrote:

https://reviews.llvm.org/D113430 landed now, which should have implemented the standard methods instead.

There is code out there, which already relies on these non-standard functions and it's going to be broken by this change without a reasonable alternative (migrating the code to C++20 is not a real alternative).

Why is migrating to C++20 not a real alternative? Just curious.

I would suggest we do this:

1. Add the methods back unless _LIBCPP_ENABLE_REMOVED_FILE_CLOCK_TIME_T_METHODS is defined.
2. Update the release note to mention that folks can use that macro to re-enable the methods temporarily, but that the option will be removed in two releases so they should move to C++20.

Whoever is broken by this will see the breakage, then read the release notes, define _LIBCPP_ENABLE_REMOVED_FILE_CLOCK_TIME_T_METHODS, and make a note to move to C++20 before they are broken for good.

In D113027#3125511, @mstorsjo wrote:

In D113027#3115431, @bgraur wrote:

This revision removes the only available methods to convert std::chrono::file_time_type to/from anything else.

The standard says std::chrono::file_time_type::clock should have methods: to_sys() and from_sys() to convert to/from std::chrono::system_time.

Is it possible to revert this change until the standard methods are implemented?

https://reviews.llvm.org/D113430 landed now, which should have implemented the standard methods instead.

Reduced the test further to:

struct k {
  ~k() __attribute__((annotate(""))) {}
};
void m() { k(); }

Reduced test case:

template <class a> void b(a);
template <typename c> class d {
public:
  class e {
  public:
    c *f();
  };
  e *g();
};
template <class> class j;
template <class h, class... i> class j<h(i...)> {
public:
  class k {
  public:
    k(int *);
    ~k();
  };
  int n();
  d<int *> l;
};
template <class h, class... i> int j<h(i...)>::n() {
  auto m = l.g()->f();
  k a(*m);
  b(a);
}
template <class h, class... i>
j<h(i...)>::k::~k() __attribute__((annotate(""))) {}
class o {
  int m_fn4();
  j<int()> p;
};
int o::m_fn4() { p.n(); }

It looks like this patch causes clang to crash on valid code with the following stack trace:

 #0 0x0000560442f12f45 SignalHandler(int)
 #1 0x00007ffac7c789a0 __restore_rt
 #2 0x0000560440083992 llvm::LazyCallGraph::Node::populateSlow()
 #3 0x0000560440082a60 llvm::LazyCallGraph::buildRefSCCs()
 #4 0x000056043fef166d llvm::ModuleToPostOrderCGSCCPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) 
 #5 0x000056043f8852f2 llvm::detail::PassModel<llvm::Module, llvm::ModuleToPostOrderCGSCCPassAdaptor, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) 
 #6 0x000056043f617204 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)
 #7 0x000056043f616e34 llvm::ModuleInlinerWrapperPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) 
 #8 0x000056043f616cb2 llvm::detail::PassModel<llvm::Module, llvm::ModuleInlinerWrapperPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) 
 #9 0x000056043f617204 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&)
#10 0x0000560440100459 (anonymous namespace)::EmitAssemblyHelper::RunOptimizationPipeline(clang::BackendAction, std::__u::unique_ptr<llvm::raw_pwrite_stream, std::__u::default_delete<llvm::raw_pwrite_stream> >&, std::__u::unique_ptr<llvm::ToolOutputFile, std::__u::default_delete<llvm::ToolOutputFile> >&)
#11 0x000056043f63dff7 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::StringRef, llvm::Module*, clang::BackendAction, std::__u::unique_ptr<llvm::raw_pwrite_stream, std::__u::default_delete<llvm::raw_pwrite_stream> >)
#12 0x00005604400f6703 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) 
#13 0x000056043f638300 clang::ParseAST(clang::Sema&, bool, bool)
#14 0x000056043f6f96d5 clang::FrontendAction::Execute()
#15 0x000056043f6f9515 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) 
#16 0x000056043f6f9381 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) 
#17 0x00005604400b84f8 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) 
#18 0x000056043f7a7a79 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&)
#19 0x000056043ff4b6f4 main
#20 0x00007ffac7ae6bbd __libc_start_main
#21 0x000056043f815679 _start

Working on a reduced test.

In D110613#3029276, @junparser wrote:

So this change blocks optimization from D106056, @alexfh would you like to workaround with -mllvm -tail-dup-indirect-size=4？

In D49864#2857630, @janosimas wrote:

That makes sense.
Should I add it somewhere? Or do I need to talk to someone?

In D106056#3024381, @junparser wrote:

@alexfh Turns out it was caused by EarlyTailDuplicatePass, would you try with -mllvm -disable-early-taildup=true?

In D106056#3024018, @junparser wrote:

In D106056#3023813, @junparser wrote:

In D106056#3023777, @xbolva00 wrote:

In D106056#3023709, @alexfh wrote:

We'd appreciate a prompt fix or a workaround. If neither is possible, please revert the commit while working on a proper solution.

Well, it is not "common" code, switch 1000 cases. But easy fix is probably to introduce some reasonable limit when to bail out.

switch 1000 cases is not the issue, use loop induction variable as condition is. It cause LVI recursive call SolveBlockValue.

@xbolva00 I was wrong. The compiler time regression is caused by CodeGen Passes rather than CVP or LVI.

We'd appreciate a prompt fix or a workaround. If neither is possible, please revert the commit while working on a proper solution.

Reduced test case:

In D106056#3021866, @xbolva00 wrote:

In D106056#3021783, @alexfh wrote:

FYI, we see a ~5x increase in compile time on some of our code (mainly on some large protobuf-generated files) after rG8ba2adcf9e54b34ba8efa73ac0d81a1192e4f614. We're working on an isolated test case.

With switches with the huge number of cases?

Repeating my question from an earlier comment: would a header glob list (similar to how the format of the -checks= flag) be enough for the use cases folks have? On the one hand glob list doesn't support all the features of regular expressions, but are they all useful for matching paths? Glob list in clang-tidy currently supports set union (similar to regex |) and <any subsequence> - * (regex .*). If needed, the support can be expanded with ?, character classes, character ranges and/or other features of POSIX globs (https://man7.org/linux/man-pages/man7/glob.7.html). On the flipside, glob list has a cleaner syntax (no need to quote characters common in paths - like .), and allows to easily express exclusion of subsets. It should be a convenient tool to represent a set of files / directories. In comparison to the proposed header-filter + exclude-header-filter glob list makes it possible to naturally express restrictions similar to "everything under a/ (except for everything under a/b/ (except for everything under a/b/c/))" - a/,-a/b/,a/b/c/.

FYI, we see a ~5x increase in compile time on some of our code (mainly on some large protobuf-generated files) after rG8ba2adcf9e54b34ba8efa73ac0d81a1192e4f614. We're working on an isolated test case.

In D105020#2999773, @ABataev wrote:

In D105020#2999765, @xbolva00 wrote:

In D105020#2999752, @ABataev wrote:

In D105020#2975046, @goncharov wrote:

Here is a repro I found

$ cat ./repro.cc
#include <iostream>
namespace a {
__attribute__((noinline)) void b(uint8_t *c, float *d, uint16_t e, uint16_t f) {
  uint16_t g = f;
  uint16_t h = c[0] << 2 | c[4] & 3;
  uint16_t l = c[1] << 2 | c[4] >> 2 & 3;
  uint16_t v = c[2] << 2 | 3;
  uint16_t j = c[3] << 2 | c[4] >> 6;
  *(d - 1) = float(l - e) / g;
  *(d - 2) = float(h - e) / g;
  *(d - 3) = float(j - e) / g;
  *(d - 4) = float(v - e) / g;
}
void bar(uint8_t *buffer, size_t k, size_t height, size_t, uint16_t e,
         uint16_t f, float *m) {
  int n = 24, g = f;
  float *t = &m[(24 - 2) * 4];
  size_t u = 2 * k;
  for (int o; o < height; o++) {
    uint8_t *p = buffer;
    uint8_t *q = p;
    float *r = &t[u];
    for (int a = 0; a < u; a += 4) {
      b(q, r, e, g);
      r -= 4;
    }
    t -= n;
  }
}
}  // namespace a
int main() {
  uint8_t s[]{255, 0, 255, 0, 51};
  float m[4 * 24]{};
  a::bar(s, 4, 4, 0, 0, 1023, m);
  int *out = reinterpret_cast<int *>(m);
  int64_t sum;
  for (int i = 0; i < sizeof(m) / sizeof(int); i++) sum += out[i];
  std::cout << sum;
}
$ clang-before -O2 -std=gnu++17 repro.cc
$ ./a.out
17045651456
$ clang-after -O2 -std=gnu++17 repro.cc
$ ./a.out
16609148640

Sorry, the reproducer is not correct. It has not initialized variables, writes after array bounds etc. Unable to use it as a reproducer for the investigation.

So if they are not able to produce UB-free reproducer, you should recommit the patch.

I believe they have a reproducer and tried to reduce it but the tool just extracted/removed too much code here :). I'll try to check manually some parts of the code and wait for the actual reproducer, will recommit the patch in a couple of days if would not be able to find a bug/no correct reproducer provided.

Some of our tests started failing after rG84cbd71c95923f9912512f3051c6ab548a99e016 (and previously after rGa28234e37af877b2b4a23c2091c27fa18c155f9a). Could you revert it again while we're working on a test case?

Thanks for the prompt fix! One nit.

In D106450#2960937, @RKSimon wrote:

Please can somebody confirm that rG10c982e0b3e6d46d1fe288d7dbe0a393c65a640f reverts the regressions you were seeing

In D106450#2960937, @RKSimon wrote:

Please can somebody confirm that rG10c982e0b3e6d46d1fe288d7dbe0a393c65a640f reverts the regressions you were seeing

I think, it's the right first step, and if there's a need to make the transformation support more use cases like checking the result of malloc for null, that can be added as a separate step.

Looks good to me!

In D106450#2956763, @RKSimon wrote:

In D106450#2956001, @alexfh wrote:

In D106450#2955083, @aeubanks wrote:

Given that we have a test case and multiple people have reported regressions, can we revert in the meantime?

Especially if there's no obvious and clear forward fix, I'd appreciate if you could unblock us by reverting for now. Thanks!

Except that regresses other benchmarks that I was addressing with this patch - bullet etc.

In D103009#2956871, @yurai007 wrote:

I found and reduced a test case that shows a too eager replacement of malloc with calloc: https://gcc.godbolt.org/z/dTjonof74
(...) This is quite a serious problem. Would you please revert while working on a fix?

First of all just to make it clear - in this particular case GCC does exactly same: https://gcc.godbolt.org/z/qbE6Kxdnv unless you pass different flags.

In D103009#2956177, @xbolva00 wrote:

Well, I am not sure, your code compiled with GCC will have same issue.

In D103009#2955983, @lebedev.ri wrote:

I think i see where this is going - the just-malloced, but never touched memory
doesn't need to be actually backed by an actual pages (see overcommit),
while i guess calloc doesn't just mark the pages as zeroed-out,
but actually marks them dirty and needed to be allocated,
at least not unless you happen to allocate in multiples of page size?

In D106450#2955083, @aeubanks wrote:

Given that we have a test case and multiple people have reported regressions, can we revert in the meantime?

In D103009#2954485, @xbolva00 wrote:

In D103009#2954430, @alexfh wrote:

This commit seems to cause memory usage (rss) increase in MariaDB's mysqld by a factor of 4. Looking back into the history, I found that the previous commit here caused the same regression, but we quickly picked up the revert and moved on. Now I'm trying to isolate the problem, but it's taking time.
So far, my sole hypothesis is that malloc + memset of a smaller size can still be converted to calloc. But I have no evidence so far. Naive attempts to synthetically reproduce this didn't work. Maybe this only happens when some UB is in place, but again, I have nothing to back this up.

Given this is a quite serious regression, can we roll this back while I'm investigating?

You should be able to use flag -fno-builtin-calloc to disable this transformation.

This transformation is correct and valid; GCC does it as well. No reason to revert, not justified. You should check asm diffs w and w/o patch for any surprises.

This commit seems to cause memory usage (rss) increase in MariaDB's mysqld by a factor of 4. Looking back into the history, I found that the previous commit here caused the same regression, but we quickly picked up the revert and moved on. Now I'm trying to isolate the problem, but it's taking time.
So far, my sole hypothesis is that malloc + memset of a smaller size can still be converted to calloc. But I have no evidence so far. Naive attempts to synthetically reproduce this didn't work. Maybe this only happens when some UB is in place, but again, I have nothing to back this up.

@RKSimon we've discovered that this commit is causing substantial performance regressions (~11% runtime) of some of our benchmarks on different x86 microarchitectures (haswell, skylake). @wmi is trying to get an isolated test.

In D104601#2916973, @Meinersbur wrote:

@romanovvlad This is due to -fms-extensions. It Expands __Pragma to #pragma instead of keeping it a __Pragma. This is a one-line fix.

@alexfh This was fixed by D106924

Looking at this once again, I'm pretty sure that reverting this is much safer than trying to fix forward. Unless there's a really trivial fix to the outstanding issues, I'll revert this later today.

In D104601#2915102, @alexfh wrote:

This commit changes the behavior of clang -E -P even when no -fminimize-whitespace is used. This breaks certain use cases like using clang to preprocess files for flex, which turns out to be sensitive to the presence of line breaks in places where C++ compilers aren't.

An isolated test case:

$ clang-old -E -x c++ -P - -o /tmp/pp.good
#define I(x, ...) \
 x { return X##x; }



#ifndef A
#define A(op, x) I(op, x)
#endif

A(foo, {







})
A(bar, {})
$ cat /tmp/pp.good
foo { return Xfoo; }
bar { return Xbar; }
$ clang-new -E -x c++ -P - -o /tmp/pp.bad
#define I(x, ...) \
 x { return X##x; }



#ifndef A
#define A(op, x) I(op, x)
#endif

A(foo, {







})
A(bar, {})
$ cat /tmp/pp.bad
foo { return Xfoo; }bar { return Xbar; }

Please fix or revert the commit.

Thanks!

This commit changes the behavior of clang -E -P even when no -fminimize-whitespace is used. This breaks certain use cases like using clang to preprocess files for flex, which turns out to be sensitive to the presence of line breaks in places where C++ compilers aren't.

In D105890#2873969, @thakis wrote:

I think this is incorrect. See D105892 and https://reviews.llvm.org/D104915#2873851 . Don't blindly land changes to suppress warnings – the warnings exist to tell us something :)

LG. I can commit the patch for you.

I have no idea what this code is doing, but just based on the fact that it fixes the problem, it looks good to me ;)

The patch fixes the segfaults we've seen after https://reviews.llvm.org/D103458.

Function-wise the patch looks good, but I'm somewhat concerned about silently breaking users. At the very least there should be a relevant entry in the release notes.

I thought I sent this batch of comments loong ago =\

BTW, any ideas why "patch application failed"? (See the Build status in the Diff Detail section)

Thanks for the great work! This is mostly looking fine, but I've added a few comments. I could only make a quite superficial review so far, but I'll try to take a deeper look into this soon.

LG with a couple of nits.

Looks good. Thanks for the fix! I'll get it landed for you.

In D97288#2581908, @sammccall wrote:

How strong is the need for this?
This adds complexity to a widely implemented and used interface, and the combination of virtual + default parameters can be at least a little confusing.

LG. Thanks for the fix!

Artem, could you set the repository to rG LLVM Github Monorepo when uploading patches as mentioned in https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface ? This way you'll allow pre-merge checks to run.

Thanks, looks good!

Apologies again for the long delay.

Thanks! Looks good now.

Looks good!

Looks good!

Trying to upload complete patch.