I don't see any messages in the reverts, but this change caused crashes both times it landed, in case that wasn't already the reason that it was reverted each time.

In rG7647cb14dcd0c471d1bc53b6a864c4737e0e82e8#997632, @dblaikie wrote:

Some detail in the commit message of a revert explaining what issues motivated the revert would be handy

• Require tsan feature to avoid buildbot jobs that don't support -fsanitize=thread (asan, windows, 32bit, ARMv7/8)

Thanks! I'll wait for another CI notification to post before landing.

• Add requires for clang to avoid unknown gcc flag errors

The test I added is not the greatest test, but it catches the issue. Lemme know if this works.

• Add a codegen test

In D102993#2811337, @teemperor wrote:

Sure I can take a look, but I don't see the immediate problem when looking at the backtrace.

I assume it takes some time to reduce the bug in Chrome? If you could get me the dump() output for the Decls D, (Decl*)ToDC, FromRD, ToD, (Decl*)ToD->getLexicalDeclContext() and whether the left or the right side of the && is true/false then maybe it becomes more obvious what's going wrong here.

This commit seems to be causing an LLDB crash. I'm still working on gathering info and reducing it, but maybe the crash reason is obvious to you given this stack trace:

In D103846#2805849, @ldionne wrote:

Is it possible to write a test for that? When you say

This was caught by tsan tests after D99434 [...]

Do you mean that any Clang that includes the fix for D99434 will catch the error with the existing libc++ test suite when TSAN is enabled? If so, then I think it's fine not to add a test, since it's already tested. We could consider moving more CI configurations to Clang trunk, too.

Requesting changes until I have answers to my questions, but otherwise this LGTM.

In D101191#2788596, @spatel wrote:

In D101191#2783570, @rupprecht wrote:

In D101191#2782963, @rupprecht wrote:

The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.

Filed as http://llvm.org/PR50500

We needed SLP to get to the problem state in that example, but the bug was in instcombine/instsimplify.
Should be fixed with:
7bb8bfa0622b

While building this, there's a warning about an unhandled switch:

It occurs on ArchiveWriter.cpp. I implement a way to write Big Archive. If you think it is better, I can add some code to remove warning, but my goal is to implement read and write for Big Archive, so it will be corrected in a future commit / PR.

Many people build with -Werror, so it would be good to handle the case even if it's explicitly ignored, like:

In D101191#2782963, @rupprecht wrote:

The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.

The issue I'm seeing seems more directly caused by SLP vectorization, as it goes away with -fno-slp-vectorize. This patch merely unblocks that bad optimization AFAICT.

FYI, I'm seeing what I think is a miscompile that bisects to this patch. Greatly simplified, the problematic snippet is this:

Thanks, this fixes the miscompile I'm seeing.

In D101555#2780152, @ABataev wrote:

In D101555#2780145, @rupprecht wrote:

We're seeing some test failures that bisected to this patch, possibly a miscompile. The test failure is in the unit test for this file: https://github.com/google/tink/blob/master/cc/subtle/aes_eax_aesni.cc. Are there already any known issues with this patch?

No, there are not. It would help if you could provide the reproducer and exact compile command to check if the problem exists.

We're seeing some test failures that bisected to this patch, possibly a miscompile. The test failure is in the unit test for this file: https://github.com/google/tink/blob/master/cc/subtle/aes_eax_aesni.cc. Are there already any known issues with this patch?

This patch introduces an assertion error we believe may be contributing to a miscompile (along with some other recent SLP patches -- this patch fixes the reduced case in http://llvm.org/PR50323, but doesn't fix the full case it was reduced from):

Updated patch LG -- the unreduced test passes now (and has no assertion errors). Thanks! (Deferring to @nikic or others for actual re-review).

In D102299#2753300, @jhenderson wrote:

Is it strictly ends with, or do things like file extensions, version numbers etc have an impact? See the additional test cases in https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-objcopy/tool-name.test.

Temporarily reverted in fec2945998947f04d672e9c5f33b57f7177474c0 to keep trunk clean.

Thanks for taking a look. Would it be all right to revert in the meantime? (I'm assuming the fix is not trivial)

We're seeing some issues with this patch, potentially a miscompile. When we enable assertions we get an error about widening atomic loads -- I'm not sure this is the source of the miscompile we're seeing, but it certainly looks related (I think this is causing corruption and leading to issues elsewhere).

$ cat repro.ll
; ModuleID = 'repro.ll'
source_filename = "repro.ll"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

I think we're all good now. The template depth fix in 84f0bb619507caf7c4809400138dae1da578282a actually exposed a long standing clang bug which needed another fix in 6bbfa0fd408e81055c360c2e059554dd76fd7f09. Thanks for the quick followups!

In D96523#2730264, @ldionne wrote:

This should fix the instantiation depth issue: https://reviews.llvm.org/D101661

@rupprecht Please ping me once you've reduced to something that doesn't involve Gmock and I'll look into it.

Another issue we're seeing is interactions between std::any and gmock -- I'll see if I can reduce it to something w/o the gmock dependency.

We're seeing some issues with this patch manifested as complicated tuple expressions that hit compiler limits with template depth. A bit of a synthetic example, but here it is: https://godbolt.org/z/TzvzWhYYs

Landed 44e2247dcd04f3421164b085094eb575270564ba to fix LLDB. If you decide to go in a different direction again, please adjust that fix accordingly.

Here's a C++ repro

I'm seeing a crash after the most recent time this was re-applied in 791930d74087b8ae8901172861a0fd21a211e436. The crash is when compiling https://github.com/beltoforion/muparser/blob/master/src/muParserTest.cpp, but I'm reducing it now, as the error might also depend on how we build that internally (we use at least`-g -O3`)

LGTM too, thanks for writing this up!

Added a few others that review binutils. Thanks for the patch!

• Add comments about memory management for m_ipd_buf

• Move comment about switch to the correct spot

• Refactor cleanup of m_cxx_method_parser

In D97786#2693410, @jingham wrote:

In D97786#2693381, @cmtice wrote:

I had to revert this change because the test case broke the windows builder. What's the right way to update/mark the test case so that it is only run on appropriate architectures & operating systems?

You add the python skipIfWindows "decorator" before each test case definition. Just grep around in the test/API directory and you'll see plenty of instances of its use.

Since this is a shell (lit) test, I think you mean adding # REQUIRES: windows at the top of the test?

The motivation for this change is that sometimes engineers misidentify the output of these messages as the cause for a test failure

@spatel c590a9880d7a660a1c911fce07f3d01ea18be2df fixes the unreduced test case. Thanks!

Here's a repro for the issue we're seeing. It only seems to be happening with msan enabled, so I'm not sure if this is just an msan bug, or a general miscompile that *happens* to trigger in this test when msan is enabled.

We're still seeing some *very* strange miscompiles as a result of this patch (maybe not really this patch's fault, it could be some bad optimization that this unblocks). I'm working on a reduction right now.

Thanks, the error message is much better now!

I don't have anything more to add now, so resigning now & leaving final thoughts and wording nits to other reviewers.

As a whole, I think this patch is going in the right direction -- we definitely need more concrete guidance on reverting patches.

Thanks!

I tried creating an IR repro, but: running -S -emit-llvm with the old PM crashes before it can generate anything, and running with the new PM seems to generate invalid IR (branches to %for.inc, which does not exist). I suspect this is not an optimizer bug, but crashing in the optimizer because invalid IR has been fed to it.

The error message I posted earlier was when using O1 + new PM, but it crashes with the old one & no optimizations:

We're seeing a crash in the optimizer after this patch, with the following logged in assert builds: assert.h assertion failed at llvm/include/llvm/Support/Casting.h:104 in static bool llvm::isa_impl_cl<llvm::InvokeInst, const llvm::Instruction *>::doit(const From *) [To = llvm::InvokeInst, From = const llvm::Instruction *]: Val && "isa<> used on a null pointer"

Here's an IR reproducer, derived from the C++ one I provided in the reverting commit (as before, it reproduces with "clang -O2", although it doesn't with "opt -O2"):

Reverted in 8d20f2c2c66eb486ff23cc3d55a53bd840b36971. Although this fixes a crash, it also introduces a compile timeout (haven't checked if it's a true infinite loop or just _really_ slow) on other code. The reproducer is in the commit message.

This fixes a build error we're seeing, so I'd like to land this in a bit if that's OK.

In D92059#2525888, @rampitec wrote:

In D92059#2525818, @rupprecht wrote:

This commit seems to be causing some internal test failures (raising a SIGFPE). I'm working on getting more details or a reduced test case to share. Is disabling this with -mllvm -slp-max-vf=1 generally OK as a short term workaround? That seems to disable enough bits that the tests pass with that, but I'm not sure if there are other implications by setting that.

Which tests are these? Setting the option to 1 effectively disables vectorization.

Verified, thanks!

FWIW, I applied this patch and was able to make it pass this pretty exhaustive test:

In D96074#2545092, @ldionne wrote:

Thanks a lot for finding this regression and reporting it. How did you find it?

We had 10-20 test failures internally that all bisected to this patch, and I logged the before/after of std::nth_element near the test failures. The values here are the relative ordering of the case that failed -- not the actual values, of course :)

When I was reducing a test case, one thing I noticed was the problem only happens for certain pivot values, e.g.:

It looks like this is generating incorrect results. I uploaded D96074 which shows the failing test case.