In rG1e0709167f5edd330889f51bb203c458bdb5e359#1187626, @steelannelida wrote:

The relanded commit caused the same problem to reappear. Reverted it in 3048c9e15498161572483e3d4b81c9c37bf2770f. Unfortunately we still don't have a reproducer. My apologies.

In D144568#4228723, @alexfh wrote:

In D144568#4227999, @philnik wrote:

In D144568#4227982, @alexfh wrote:

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

I figured out. The test is really a corner case and it's not representative of real code. It artificially creates a chain of 1024 recursive template instantiations and the deepest level started requiring one more template instantiation for std::string after this patch. This is definitely not a problem elsewhere.

In D144568#4227999, @philnik wrote:

In D144568#4227982, @alexfh wrote:

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

Was this sort of a compiler time regression anticipated with for this change?

I didn't expect really any change w.r.t. compile time. I also don't see how it would affect the amount of recursive template instantiations, since it doesn't add any layers. It just moves the instantiations into the type list. Do you have any specific cases where it changes?

This commit increases compilation time by ~15% and increases recursive template instantiation depth (thus, requiring adjusting -ftemplate-depth=2048) in some of our internal test code. Admittedly, it's using templates in a sort of a compiler-load-test-like way, but this change may affect compilation time for real code as well (it's just not likely to hit the default -ftemplate-depth=1024 limit and I don't have a good idea of how to find this code with reasonable effort).

In D144262#4199920, @Mordante wrote:

This is a known issue, see this bug report https://bugs.mysql.com/bug.php?id=110254

The following snippet (reduced from a version of mysql) compiles well before this commit and fails to compile after it (https://gcc.godbolt.org/z/zrGbbbWb5):

In D143840#4147582, @joanahalili wrote:

In D143840#4147234, @joanahalili wrote:

Heads up: We are experiencing a series of clang crashes because of this commit.

What we already have so far:

Unhandled DeclRefExpr
UNREACHABLE executed at clang/lib/CodeGen/CGExpr.cpp:2958!

We are now working on a reproducer.

Here is our reproducer:

struct a {
  enum { b };
};
struct c {
  void *d;
};
struct e {
  void f(void *);
};
template <typename g> struct h : g {
  h(short, int);
  virtual void i();
};
template <typename g> void h<g>::i() { e().f(c::d); }
struct j : h<c> {
  j();
};
int k;
j::j() : h(a::b, k) {}

In D135462#4125826, @alexfh wrote:

In D135462#4125805, @alexfh wrote:

In D135462#4115559, @asavonic wrote:

The patch is merged (again). Please let me know if it causes any regressions for in-tree targets or tests.

An early heads up: we started seeing some test failures after this commit, but I can't yet tell whether the commit or the code is at fault.

The only data point so far is that failures are happening on AArch64, which is alignment-sensitive.

In D135462#4125805, @alexfh wrote:

In D135462#4115559, @asavonic wrote:

The patch is merged (again). Please let me know if it causes any regressions for in-tree targets or tests.

An early heads up: we started seeing some test failures after this commit, but I can't yet tell whether the commit or the code is at fault.

In D135462#4115559, @asavonic wrote:

The patch is merged (again). Please let me know if it causes any regressions for in-tree targets or tests.

The implementation with two different class names seems much cleaner. Thanks!

In D133661#4096636, @philnik wrote:

I've uploaded D143071 to fix the problem. I talk to Louis later today, so a fix should be commited within the next 6 hours or so.

In D133661#4096263, @alexfh wrote:

@philnik could you commit one of the proposed abi_tag fixes?

@philnik could you commit one of the proposed abi_tag fixes?

In D133661#4095119, @rupprecht wrote:

The ABI tag approach fixes the ODR issue I was looking at

@@ -60,7 +60,7 @@

 #ifndef _LIBCPP_NO_EXCEPTIONS
 template <class _Rollback>
-struct __exception_guard {
+struct __attribute__((__abi_tag__(("fexceptions")))) __exception_guard {
   __exception_guard() = delete;

   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 explicit __exception_guard(_Rollback __rollback)
@@ -89,7 +89,7 @@
 };
 #else  // _LIBCPP_NO_EXCEPTIONS
 template <class _Rollback>
-struct __exception_guard {
+struct __attribute__((__abi_tag__(("fnoexceptions")))) __exception_guard  {
   __exception_guard() = delete;
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NODEBUG explicit __exception_guard(_Rollback) {}

In D133661#4093307, @philnik wrote:

In D133661#4093298, @bgraur wrote:

We're seeing a lot of fallout from this patch and they all look related to the ODR violations that seem to be intentionally added here: both the patch description and the comment for the __exception_guard mention explicitly that combinations of code compiled with exceptions and without exceptions are common.

Am I missing something here?

Do you have a specific example?

We've found the cause of the problems @asbirlea reported above: violation of aliasing rules. Something similar to this snippet: https://gcc.godbolt.org/z/3Y9GEMW4e

In D140158#4082804, @jhuber6 wrote:

In D140158#4082789, @alexfh wrote:

This patch breaks our cuda compilations. The output file isn't created after it:

$ echo 'extern "C" __attribute__((global)) void q() {}' >q.cc
$ good-clang \
    -nocudainc -x cuda \
    --cuda-path=somepath/cuda/ \
    -Wno-unknown-cuda-version --cuda-device-only \
    -c q.cc -o qqq-good.o \
  && bad-clang \
    -nocudainc -x cuda \
    --cuda-path=somepath/cuda/ \
    -Wno-unknown-cuda-version --cuda-device-only \
    -c q.cc -o qqq-bad.o \
  && ls qqq*.o
qqq-good.o

https://github.com/llvm/llvm-project/issues/60301 Still broken after this fix?

This patch breaks our cuda compilations. The output file isn't created after it:

$ echo 'extern "C" __attribute__((global)) void q() {}' >q.cc
$ good-clang \
    -nocudainc -x cuda \
    --cuda-path=somepath/cuda/ \
    -Wno-unknown-cuda-version --cuda-device-only \
    -c q.cc -o qqq-good.o \
  && bad-clang \
    -nocudainc -x cuda \
    --cuda-path=somepath/cuda/ \
    -Wno-unknown-cuda-version --cuda-device-only \
    -c q.cc -o qqq-bad.o \
  && ls qqq*.o
qqq-good.o

Thanks for preparing this fix! I've verified that it fixes the original reproducer as well.

In rG12cb1cb3720de8d164196010123ce1a8901d8122#1167977, @erichkeane wrote:

DO NOT revert this, doing so causes about 1/2 our CI to stop working.

It looks like there's no easy fix for now, right? I'm about to revert this commit (12cb1cb3720de8d164196010123ce1a8901d8122) to unblock us.

There's another interesting issue (reduced from https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/stringpiece_8h_source.html) I bisected to this commit: https://gcc.godbolt.org/z/W5x1s9KbK

In D140939#4072755, @goldstein.w.n wrote:

In D140939#4072672, @goldstein.w.n wrote:

In D140939#4072488, @alexfh wrote:

In D140939#4072474, @alexfh wrote:

In D140939#4072473, @alexfh wrote:

Unfortunately, there's another problem, which doesn't get fixed by https://reviews.llvm.org/D142166 / 2e25204779e5b972d668bf66a0014c1325813b35:

assert.h assertion failed at llvm/lib/Target/X86/X86ISelLowering.cpp:31466 in std::pair<Value *, BitTestKind> FindSingleBitChange(Value *): I != nullptr
    @     0x56257abc9524  __assert_fail
    @     0x562578c7fd1c  FindSingleBitChange()
    @     0x562578c7f6dd  llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR()
    @     0x562578c8098b  llvm::X86TargetLowering::shouldExpandAtomicRMWInIR()
    @     0x5625790a6099  (anonymous namespace)::AtomicExpand::tryExpandAtomicRMW()
    @     0x5625790a59dd  (anonymous namespace)::AtomicExpand::runOnFunction()
    @     0x56257a86e87d  llvm::FPPassManager::runOnFunction()
    @     0x56257a876104  llvm::FPPassManager::runOnModule()
    @     0x56257a86ef7c  llvm::legacy::PassManagerImpl::run()
    @     0x562575bad581  clang::EmitBackendOutput()
    @     0x562575baabe9  clang::BackendConsumer::HandleTranslationUnit()
    @     0x562576a85e5c  clang::ParseAST()
    @     0x5625767cbf23  clang::FrontendAction::Execute()
    @     0x562576741cad  clang::CompilerInstance::ExecuteAction()
    @     0x5625757871e8  clang::ExecuteCompilerInvocation()
    @     0x56257577ae41  cc1_main()
    @     0x562575776ec8  ExecuteCC1Tool()
    @     0x5625768ed317  llvm::function_ref<>::callback_fn<>()
    @     0x56257aa23d20  llvm::CrashRecoveryContext::RunSafely()
    @     0x5625768ecb63  clang::driver::CC1Command::Execute()
    @     0x5625768ab2e6  clang::driver::Compilation::ExecuteCommand()
    @     0x5625768ab60f  clang::driver::Compilation::ExecuteJobs()
    @     0x5625768caf70  clang::driver::Driver::ExecuteCompilation()
    @     0x562575776027  clang_main()
    @     0x7f8f04be3633  __libc_start_main
    @     0x562575772bea  _start

I'd suggest to revert while investigating. I'm working on an isolated test case.

$ cat reduced.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"

define weak_odr void @f() {
entry:
  br label %if.end19

cont11:                                           ; No predecessors!
  %not = xor i32 0, -1
  %0 = atomicrmw and ptr null, i32 %not monotonic, align 4
  %and13 = and i32 %0, 0
  br label %if.end19

if.end19:                                         ; preds = %cont11, %entry
  ret void
}
$ ./good-clang -c reduced.ll -o good.o
$ ./bad-clang -c reduced.ll -o bad.o
assert.h assertion failed at llvm/lib/Target/X86/X86ISelLowering.cpp:31466 in std::pair<Value *, BitTestKind> FindSingleBitChange(Value *): I != nullptr
    @     0x559d015c9524  __assert_fail
    @     0x559cff67fd1c  FindSingleBitChange()
    @     0x559cff67f6dd  llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR()
    @     0x559cff68098b  llvm::X86TargetLowering::shouldExpandAtomicRMWInIR()
    @     0x559cffaa6099  (anonymous namespace)::AtomicExpand::tryExpandAtomicRMW()
    @     0x559cffaa59dd  (anonymous namespace)::AtomicExpand::runOnFunction()
    @     0x559d0126e87d  llvm::FPPassManager::runOnFunction()
    @     0x559d01276104  llvm::FPPassManager::runOnModule()
    @     0x559d0126ef7c  llvm::legacy::PassManagerImpl::run()
    @     0x559cfc5ad581  clang::EmitBackendOutput()
    @     0x559cfc5a94f8  clang::CodeGenAction::ExecuteAction()
    @     0x559cfd1cbf23  clang::FrontendAction::Execute()
    @     0x559cfd141cad  clang::CompilerInstance::ExecuteAction()
    @     0x559cfc1871e8  clang::ExecuteCompilerInvocation()
    @     0x559cfc17ae41  cc1_main()

Thank you for finding the case. A another one (and probably more realistic one is):

define weak_odr void @f(i32 %0) {
entry:
  br label %if.end19

cont11:                                           ; No predecessors!
  %not = xor i32 %0, -1
  %1 = atomicrmw and ptr null, i32 %not monotonic, align 4
  %and13 = and i32 %1, 0
  br label %if.end19

if.end19:                                         ; preds = %cont11, %entry
  ret void
}

So the fix is:

-      assert(I != nullptr);
+
+      // If constant it will fold and we can evaluate later. If its an argument
+      // or something of that nature, we can't analyze.
+      if (I == nullptr)
+        return {nullptr, UndefBit};

I'm happy to post that as a patch.

Created: https://reviews.llvm.org/D142339

But let me know if revert is still preferable (I say as much there).

If you still want to revert, however, happy to do so as well and then repost the full version.

Whichever you think is more prudent.

In D140939#4072474, @alexfh wrote:

In D140939#4072473, @alexfh wrote:

Unfortunately, there's another problem, which doesn't get fixed by https://reviews.llvm.org/D142166 / 2e25204779e5b972d668bf66a0014c1325813b35:

assert.h assertion failed at llvm/lib/Target/X86/X86ISelLowering.cpp:31466 in std::pair<Value *, BitTestKind> FindSingleBitChange(Value *): I != nullptr
    @     0x56257abc9524  __assert_fail
    @     0x562578c7fd1c  FindSingleBitChange()
    @     0x562578c7f6dd  llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR()
    @     0x562578c8098b  llvm::X86TargetLowering::shouldExpandAtomicRMWInIR()
    @     0x5625790a6099  (anonymous namespace)::AtomicExpand::tryExpandAtomicRMW()
    @     0x5625790a59dd  (anonymous namespace)::AtomicExpand::runOnFunction()
    @     0x56257a86e87d  llvm::FPPassManager::runOnFunction()
    @     0x56257a876104  llvm::FPPassManager::runOnModule()
    @     0x56257a86ef7c  llvm::legacy::PassManagerImpl::run()
    @     0x562575bad581  clang::EmitBackendOutput()
    @     0x562575baabe9  clang::BackendConsumer::HandleTranslationUnit()
    @     0x562576a85e5c  clang::ParseAST()
    @     0x5625767cbf23  clang::FrontendAction::Execute()
    @     0x562576741cad  clang::CompilerInstance::ExecuteAction()
    @     0x5625757871e8  clang::ExecuteCompilerInvocation()
    @     0x56257577ae41  cc1_main()
    @     0x562575776ec8  ExecuteCC1Tool()
    @     0x5625768ed317  llvm::function_ref<>::callback_fn<>()
    @     0x56257aa23d20  llvm::CrashRecoveryContext::RunSafely()
    @     0x5625768ecb63  clang::driver::CC1Command::Execute()
    @     0x5625768ab2e6  clang::driver::Compilation::ExecuteCommand()
    @     0x5625768ab60f  clang::driver::Compilation::ExecuteJobs()
    @     0x5625768caf70  clang::driver::Driver::ExecuteCompilation()
    @     0x562575776027  clang_main()
    @     0x7f8f04be3633  __libc_start_main
    @     0x562575772bea  _start

I'd suggest to revert while investigating. I'm working on an isolated test case.

In D140939#4072473, @alexfh wrote:

Unfortunately, there's another problem, which doesn't get fixed by https://reviews.llvm.org/D142166 / 2e25204779e5b972d668bf66a0014c1325813b35:

assert.h assertion failed at llvm/lib/Target/X86/X86ISelLowering.cpp:31466 in std::pair<Value *, BitTestKind> FindSingleBitChange(Value *): I != nullptr
    @     0x56257abc9524  __assert_fail
    @     0x562578c7fd1c  FindSingleBitChange()
    @     0x562578c7f6dd  llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR()
    @     0x562578c8098b  llvm::X86TargetLowering::shouldExpandAtomicRMWInIR()
    @     0x5625790a6099  (anonymous namespace)::AtomicExpand::tryExpandAtomicRMW()
    @     0x5625790a59dd  (anonymous namespace)::AtomicExpand::runOnFunction()
    @     0x56257a86e87d  llvm::FPPassManager::runOnFunction()
    @     0x56257a876104  llvm::FPPassManager::runOnModule()
    @     0x56257a86ef7c  llvm::legacy::PassManagerImpl::run()
    @     0x562575bad581  clang::EmitBackendOutput()
    @     0x562575baabe9  clang::BackendConsumer::HandleTranslationUnit()
    @     0x562576a85e5c  clang::ParseAST()
    @     0x5625767cbf23  clang::FrontendAction::Execute()
    @     0x562576741cad  clang::CompilerInstance::ExecuteAction()
    @     0x5625757871e8  clang::ExecuteCompilerInvocation()
    @     0x56257577ae41  cc1_main()
    @     0x562575776ec8  ExecuteCC1Tool()
    @     0x5625768ed317  llvm::function_ref<>::callback_fn<>()
    @     0x56257aa23d20  llvm::CrashRecoveryContext::RunSafely()
    @     0x5625768ecb63  clang::driver::CC1Command::Execute()
    @     0x5625768ab2e6  clang::driver::Compilation::ExecuteCommand()
    @     0x5625768ab60f  clang::driver::Compilation::ExecuteJobs()
    @     0x5625768caf70  clang::driver::Driver::ExecuteCompilation()
    @     0x562575776027  clang_main()
    @     0x7f8f04be3633  __libc_start_main
    @     0x562575772bea  _start

Unfortunately, there's another problem, which doesn't get fixed by https://reviews.llvm.org/D142166 / 2e25204779e5b972d668bf66a0014c1325813b35:

assert.h assertion failed at llvm/lib/Target/X86/X86ISelLowering.cpp:31466 in std::pair<Value *, BitTestKind> FindSingleBitChange(Value *): I != nullptr
    @     0x56257abc9524  __assert_fail
    @     0x562578c7fd1c  FindSingleBitChange()
    @     0x562578c7f6dd  llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR()
    @     0x562578c8098b  llvm::X86TargetLowering::shouldExpandAtomicRMWInIR()
    @     0x5625790a6099  (anonymous namespace)::AtomicExpand::tryExpandAtomicRMW()
    @     0x5625790a59dd  (anonymous namespace)::AtomicExpand::runOnFunction()
    @     0x56257a86e87d  llvm::FPPassManager::runOnFunction()
    @     0x56257a876104  llvm::FPPassManager::runOnModule()
    @     0x56257a86ef7c  llvm::legacy::PassManagerImpl::run()
    @     0x562575bad581  clang::EmitBackendOutput()
    @     0x562575baabe9  clang::BackendConsumer::HandleTranslationUnit()
    @     0x562576a85e5c  clang::ParseAST()
    @     0x5625767cbf23  clang::FrontendAction::Execute()
    @     0x562576741cad  clang::CompilerInstance::ExecuteAction()
    @     0x5625757871e8  clang::ExecuteCompilerInvocation()
    @     0x56257577ae41  cc1_main()
    @     0x562575776ec8  ExecuteCC1Tool()
    @     0x5625768ed317  llvm::function_ref<>::callback_fn<>()
    @     0x56257aa23d20  llvm::CrashRecoveryContext::RunSafely()
    @     0x5625768ecb63  clang::driver::CC1Command::Execute()
    @     0x5625768ab2e6  clang::driver::Compilation::ExecuteCommand()
    @     0x5625768ab60f  clang::driver::Compilation::ExecuteJobs()
    @     0x5625768caf70  clang::driver::Driver::ExecuteCompilation()
    @     0x562575776027  clang_main()
    @     0x7f8f04be3633  __libc_start_main
    @     0x562575772bea  _start

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1162348, @alexfh wrote:

We're still actively working on this and so far we had to revert all the five commits locally while we're trying to find a solution. It would still help us, if it was a single commit, not five of them (i.e. I'd appreciate, if you could revert them either together or one by one, and then reland them as a single commit), but that's just for our convenience. I'll also understand, if you reject, since, well, there's nothing wrong with the code.

In D139275#4022050, @lebedev.ri wrote:

In D139275#4022015, @apilipenko wrote:

FYI, this change caused widespread performance regressions in our testing. We haven't investigated further since the change was reverted.

I can re-run these tests and do some analysis on the next revision once you decide to reapply.

At this point this is waiting on @alexfh's reproducer mostly.

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1163170, @alexfh wrote:

My little investigation shows that most of our problems were fixed at 07ecdd9b1a8af51f07d5f4dfe46845c801482a39, but some were fixed even before that.

My little investigation shows that most of our problems were fixed at 07ecdd9b1a8af51f07d5f4dfe46845c801482a39, but some were fixed even before that.

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1163148, @alexfh wrote:

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1160877, @lebedev.ri wrote:

@alexfh

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1158810, @alexfh wrote:

It took significantly longer than I expected, but this is what I came up with. The code below changes behavior when compiled with this commit:

$ cat reduced.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"

%"class.std::__u::basic_ostream" = type { ptr, %"class.std::__u::basic_ios.base" }
%"class.std::__u::basic_ios.base" = type <{ %"class.std::__u::ios_base", ptr, i32 }>
%"class.std::__u::ios_base" = type { ptr, i32, i64, i64, i32, i32, ptr, ptr, ptr, ptr, i64, i64, ptr, i64, i64, ptr, i64, i64 }

@_ZNSt3__u4coutE = external global %"class.std::__u::basic_ostream"

define i32 @main(i1 %0, i1 %1) {
  %3 = xor i1 %0, %1
  %4 = or i1 %1, %0
  %5 = select i1 %3, i64 5950, i64 0
  %6 = select i1 %4, i64 %5, i64 1
  %7 = trunc i64 %6 to i32
  %8 = tail call ptr @_ZNSt3__u13basic_ostreamIcNS_11char_traitsIcEEElsEi(ptr @_ZNSt3__u4coutE, i32 %7)
  ret i32 0
}

declare ptr @_ZNSt3__u13basic_ostreamIcNS_11char_traitsIcEEElsEi(ptr, i32)

$ good-clang reduced.ll -O3 -static-libgcc ./libc++.a -o q1 && ./q1
5950
$ bad-clang reduced.ll -O3 -static-libgcc ./libc++.a -o q1 && ./q1
0

Please let me know if this is enough.

*THANK YOU*
But, it appears i have not-great news: if i just revert d73383c145ea83d25063246e0c34f5a41fd35293, this miscompile does not happen.
BUT if i additionally revert the 07ecdd9b1a8af51f07d5f4dfe46845c801482a39, which was a fix for

@joanahalili has posted the reproducer for the problems with the original commit on Dec 16th: https://reviews.llvm.org/D139275#4001580.

then it's back.

So in other words, i believe the miscompile you were seeing was already fixed.
Would you like to double-check that the issue you were seeing does not reappear on main with revert of d73383c145ea83d25063246e0c34f5a41fd35293?

Just reverting d73383c145ea83d25063246e0c34f5a41fd35293 (on top of ac1ffd3caca12c254e0b8c847aa8ce8e51b6cfbf) doesn't show any of the problems we found after 9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1160877. So, I guess, it maybe due to the fact that 428f36401b1b695fd501ebfdc8773bed8ced8d4e was reverted and is not yet in the ToT?

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1160877, @lebedev.ri wrote:

@alexfh

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1158810, @alexfh wrote:

It took significantly longer than I expected, but this is what I came up with. The code below changes behavior when compiled with this commit:

$ cat reduced.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"

%"class.std::__u::basic_ostream" = type { ptr, %"class.std::__u::basic_ios.base" }
%"class.std::__u::basic_ios.base" = type <{ %"class.std::__u::ios_base", ptr, i32 }>
%"class.std::__u::ios_base" = type { ptr, i32, i64, i64, i32, i32, ptr, ptr, ptr, ptr, i64, i64, ptr, i64, i64, ptr, i64, i64 }

@_ZNSt3__u4coutE = external global %"class.std::__u::basic_ostream"

define i32 @main(i1 %0, i1 %1) {
  %3 = xor i1 %0, %1
  %4 = or i1 %1, %0
  %5 = select i1 %3, i64 5950, i64 0
  %6 = select i1 %4, i64 %5, i64 1
  %7 = trunc i64 %6 to i32
  %8 = tail call ptr @_ZNSt3__u13basic_ostreamIcNS_11char_traitsIcEEElsEi(ptr @_ZNSt3__u4coutE, i32 %7)
  ret i32 0
}

declare ptr @_ZNSt3__u13basic_ostreamIcNS_11char_traitsIcEEElsEi(ptr, i32)

$ good-clang reduced.ll -O3 -static-libgcc ./libc++.a -o q1 && ./q1
5950
$ bad-clang reduced.ll -O3 -static-libgcc ./libc++.a -o q1 && ./q1
0

Please let me know if this is enough.

*THANK YOU*
But, it appears i have not-great news: if i just revert d73383c145ea83d25063246e0c34f5a41fd35293, this miscompile does not happen.
BUT if i additionally revert the 07ecdd9b1a8af51f07d5f4dfe46845c801482a39, which was a fix for

@joanahalili has posted the reproducer for the problems with the original commit on Dec 16th: https://reviews.llvm.org/D139275#4001580.

then it's back.

So in other words, i believe the miscompile you were seeing was already fixed.
Would you like to double-check that the issue you were seeing does not reappear on main with revert of d73383c145ea83d25063246e0c34f5a41fd35293?

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1162048, @lebedev.ri wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1161403, @alexfh wrote:

Okay, figured out: reverting just this commit doesn't fix our issue, but reverting this commit and in addition four later related commits makes our code work again:

• [InstCombine] Try to sink not of one operand of logical operation into another hand (9f0c9e4725f6a6f1cad857102dc70fce759e610d)
• [InstCombine] Support sinking not into logical operand with invertible hands (b20ccccda22b1465b54bd069bd53a0a96224f3ec)
• [InstCombine] sinkNotIntoOtherHandOfLogicalOp(): allow extra invertible uses of hand-to-invert (6adeec881a830c91d42077b9f4665531003143b1)
• [InstCombine] sinkNotIntoOtherHandOfLogicalOp(): don't forget to re-set insert position (3ae00753c156d70b3e7000451ac71391241ac5dd)

But again, these five commits can't be reverted cleanly, since there were multiple other changes to the code and tests. It would be nice, if you could do this. We're actively working on an isolated test case and trying to understand what exactly is wrong. So far we haven't found a clear change in the semantic of the generated PTX code, but there's definitely something fishy (maybe further down the road - in ptxas or even in the hardware).

Sorry for lack of feedback here.
Before i do that, could you please try if this affects the outcome:

diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 48f00fac2d0d..fd3943283e99 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -948,7 +948,8 @@ Value *InstCombinerImpl::SimplifySelectsFeedingBinaryOp(BinaryOperator &I,
 /// Freely adapt every user of V as-if V was changed to !V.
 /// WARNING: only if canFreelyInvertAllUsersOf() said this can be done.
 void InstCombinerImpl::freelyInvertAllUsersOf(Value *I, Value *IgnoredUser) {
-  for (User *U : make_early_inc_range(I->users())) {
+  SmallVector<User*> Users(I->user_begin(), I->user_end());
+  for (User *U : Users) {
     if (U == IgnoredUser)
       continue; // Don't consider this user.
     switch (cast<Instruction>(U)->getOpcode()) {
@@ -962,6 +963,7 @@ void InstCombinerImpl::freelyInvertAllUsersOf(Value *I, Value *IgnoredUser) {
       cast<BranchInst>(U)->swapSuccessors(); // swaps prof metadata too
       break;
     case Instruction::Xor:
+      assert(match(U, m_Not(m_Value())));
       replaceInstUsesWith(cast<Instruction>(*U), I);
       break;
     default:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1161344, @alexfh wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160992, @alexfh wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160872, @lebedev.ri wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160870, @alexfh wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160823, @alexfh wrote:

r8 = (r5 != 0) ? 3 : 0;
if ((r5 != 0) || p1) goto L__BB0_5;

to

r8 = (r5 != 0) ? 3 : 0;
if ((r5 | r1) != 0) goto L__BB0_5;

Actually, that may be over-reduced and likely doesn't demonstrate a problem =\

If you are *sure* this happens on current main, please feel free to revert in the meanwhile.
That particular branch merge thing happens in simplifycfg, so i kinda expect that the change only exposes issue elsewhere.

I'm sure something wrong happens after this patch (and isn't fixed up to a quite recent LLVM commit). I'm also quite confident this affects the nvidia target (just verified that the host compilation isn't affected for the test in question). However, I can't explain what exactly the problem is. And I'm afraid it's not easy for me to revert this commit at this point - it needs to be done quite carefully due to the multiple changes to llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp that were made afterwards. If you can do this, that would help us a lot.

Please hold on. No need to revert this yet. I might have found some issues with my analysis. I'll be back with more details a bit later.

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160992, @alexfh wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160872, @lebedev.ri wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160870, @alexfh wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160823, @alexfh wrote:

r8 = (r5 != 0) ? 3 : 0;
if ((r5 != 0) || p1) goto L__BB0_5;

to

r8 = (r5 != 0) ? 3 : 0;
if ((r5 | r1) != 0) goto L__BB0_5;

Actually, that may be over-reduced and likely doesn't demonstrate a problem =\

If you are *sure* this happens on current main, please feel free to revert in the meanwhile.
That particular branch merge thing happens in simplifycfg, so i kinda expect that the change only exposes issue elsewhere.

I'm sure something wrong happens after this patch (and isn't fixed up to a quite recent LLVM commit). I'm also quite confident this affects the nvidia target (just verified that the host compilation isn't affected for the test in question). However, I can't explain what exactly the problem is. And I'm afraid it's not easy for me to revert this commit at this point - it needs to be done quite carefully due to the multiple changes to llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp that were made afterwards. If you can do this, that would help us a lot.

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160872, @lebedev.ri wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160870, @alexfh wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160823, @alexfh wrote:

r8 = (r5 != 0) ? 3 : 0;
if ((r5 != 0) || p1) goto L__BB0_5;

to

r8 = (r5 != 0) ? 3 : 0;
if ((r5 | r1) != 0) goto L__BB0_5;

Actually, that may be over-reduced and likely doesn't demonstrate a problem =\

If you are *sure* this happens on current main, please feel free to revert in the meanwhile.
That particular branch merge thing happens in simplifycfg, so i kinda expect that the change only exposes issue elsewhere.

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160823, @alexfh wrote:

r8 = (r5 != 0) ? 3 : 0;
if ((r5 != 0) || p1) goto L__BB0_5;

to

r8 = (r5 != 0) ? 3 : 0;
if ((r5 | r1) != 0) goto L__BB0_5;

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160849, @lebedev.ri wrote:

Let me take a look...
Is that with the "D139275: [SimplifyCFG] FoldBranchToCommonDest(): deal with mismatched IV's in PHI's in common successor block" reverted?

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160782, @lebedev.ri wrote:

In rG4def99e642806f3f8e94cb2aa9356d63cd233dd0#1160775, @alexfh wrote:

Heads up: I suspect that this commit breaks the nvptx64-nvidia-cuda target. I have a few cuda files that stop working correctly after this commit, and I'm reducing one of the tests.

Will take a look the moment there is a repro.
You are sure this is still happening in main, right?
There was recently a fix in this area - 30af2e31917021f68a3d16ee71335f048c9e6235

Heads up: I suspect that this commit breaks the nvptx64-nvidia-cuda target. I have a few cuda files that stop working correctly after this commit, and I'm reducing one of the tests.

In D140100#4027018, @ABataev wrote:

Must be fixed in a1b18946f9af130a14655721653beb4510fde9be

In D140100#4026269, @ABataev wrote:

In D140100#4026248, @alexfh wrote:

In D140100#4024152, @ABataev wrote:

In D140100#4024147, @alexfh wrote:

In D140100#4018269, @asmok-g wrote:

Heads-up: This has caused a change in behavior when using #pragma unroll in some test using SIMD. In other words, using #pragma unroll results in a different output than without it. I'm preparing a reduced test case.

I've got a reduced IR file for that case:

q.ll11 KBDownload

$ old-clang -O2 q.ll && ./a.out
0.0
$ new-clang -O2 q.ll && ./a.out
5.0

Please fix or revert the patch, if there's no trivial fix.

Thanks!

Thanks for the reproducer, will investigate it.

Alexey, could you revert the commit in the meantime?

It is not so easy, there are several commits on top of it already. I just started looking at it, hope to provide a fix in 1-2hrs, if it will take more time, I'll revert it.

In D140100#4024152, @ABataev wrote:

In D140100#4024147, @alexfh wrote:

In D140100#4018269, @asmok-g wrote:

Heads-up: This has caused a change in behavior when using #pragma unroll in some test using SIMD. In other words, using #pragma unroll results in a different output than without it. I'm preparing a reduced test case.

I've got a reduced IR file for that case:

q.ll11 KBDownload

$ old-clang -O2 q.ll && ./a.out
0.0
$ new-clang -O2 q.ll && ./a.out
5.0

Please fix or revert the patch, if there's no trivial fix.

Thanks!

Thanks for the reproducer, will investigate it.

In D140100#4018269, @asmok-g wrote:

Heads-up: This has caused a change in behavior when using #pragma unroll in some test using SIMD. In other words, using #pragma unroll results in a different output than without it. I'm preparing a reduced test case.

It took significantly longer than I expected, but this is what I came up with. The code below changes behavior when compiled with this commit:

In rG9ddff66d0c9c3e18d56e6b20aa26a2a8cdfb6d2b#1157486, @lebedev.ri wrote:

I've gone ahead and reverted both potential problem candidates,
WITH implicit assumption that an actionable reproducer will be provided.
Thanks.

Hi Roman, this might be causing some miscompiles in our code. I'm now trying to reduce the test case. I'll notify you when I have more information.

In D139275#4001603, @lebedev.ri wrote:

In D139275#4001601, @alexfh wrote:

I will revert this patch. Please ensure the provided test case works fine before recommitting.

Thanks for the reproducer. Will revert in a sec.

I will revert this patch. Please ensure the provided test case works fine before recommitting.

There's another problem with this patch. Please ensure it's fixed before recommitting. Clang hangs while compiling the following short snippet reduced from an open-source library (compiled on x86-64, linux):

There's a problem in the previous iteration of this patch (https://reviews.llvm.org/D138774). It made clang hang while compiling the following short snippet reduced from an open-source library (compiled on x86-64, linux):

$ cat q.c
char *texsubexpr(char *expression, char *subexpr) {
  char *texchar();
  if (*expression) return texchar(expression, subexpr);
}

Rebased.

LGTM modulo open comments.

Heads up: this commit has broken compilation in a small number of cases: some #includes from modularized headers now fail to be found. I'm still trying to figure out what exactly happened, but some behavior has definitely changed by this commit.

And another problem with this patch: there's another pattern (or multiple different patterns?) in the code, that result in around 3x clang memory usage increase after this patch. The result of -print-stats doesn't make it clear where the additional allocations come from:

Thanks for adding this diagnostic! I wonder whether we can include SLoc usage information into -print-stats output?

In D136565#3913932, @mizvekov wrote:

@alexfh Thanks!

While there is a huge increase in the amount of UsingTypes, it seems the total amount is still reasonable and does not explain the perf hit.

Perhaps this is a case of bad hashing and they are all falling into the same bucket?

cc @sam.mcall for awareness of UsingType issue.
It may be some very simple problem, I just can't even look at the code right now.

Given the broad impact of this in our code I'm inclined to revert the patch
to unblock us. The test case I have so far is still too large, but I hope
to get something shareable tomorrow.

In D136565#3913065, @mizvekov wrote:

In D136565#3911884, @alexfh wrote:

Hi Matheus, 279fe6281d2ca5b2318c7437316c28750feaac8d causes compilation timeout on some of our internal files.

Hi @alexfh. It looks like somehow we may be creating a crazy amount of extra UsingTypes in your test scenario.

There is a clang flag that prints some performance statistics, including the amount of AST nodes created. I can't lookup the spelling as I am on vacations / on a cellphone for the next two weeks, but I believe this is documented.

One idea to get a reduction here is to perhaps tie your creduce interestingness test to that UsingType count.

Hi Matheus, 279fe6281d2ca5b2318c7437316c28750feaac8d causes compilation timeout on some of our internal files. We're trying to get a test case we can share, but so far the only information I can provide is compiler perf profile difference:

Undo LGTM, since we decided to go with a revert.

FYI, I've reverted f83347b0bedb22ea676861c8e4e2ed9c31371ade and 74e4f778cf16cbf7163b5c6de6027a43f5e9169f.

In D136539#3877872, @sammccall wrote:

This is a subtle change that needs careful review, and honestly should have a test.

I realize there's a breakage that needs to be fixed with some urgency and you believe you're just restoring the old behavior, however in that case the right course of action is to revert the patch.

LG, thanks!

We started seeing incorrect behavior in some code after this commit. The test case is here: https://gcc.godbolt.org/z/hhj64noKs.