[PATCH] D140939: [X86] Transform AtomicRMW logic operations to BT{R|C|S} if only changing/testing a single bit.

Mon Jan 23 11:27:59 PST 2023

goldstein.w.n added a comment.

In D140939#4072961 <https://reviews.llvm.org/D140939#4072961>, @alexfh wrote:

> In D140939#4072755 <https://reviews.llvm.org/D140939#4072755>, @goldstein.w.n wrote:
>
>> In D140939#4072672 <https://reviews.llvm.org/D140939#4072672>, @goldstein.w.n wrote:
>>
>>> In D140939#4072488 <https://reviews.llvm.org/D140939#4072488>, @alexfh wrote:
>>>
>>>> In D140939#4072474 <https://reviews.llvm.org/D140939#4072474>, @alexfh wrote:
>>>>
>>>>> In D140939#4072473 <https://reviews.llvm.org/D140939#4072473>, @alexfh wrote:
>>>>>
>>>>>> Unfortunately, there's another problem, which doesn't get fixed by https://reviews.llvm.org/D142166 / 2e25204779e5b972d668bf66a0014c1325813b35 <https://reviews.llvm.org/rG2e25204779e5b972d668bf66a0014c1325813b35>:
>>>>>>
>>>>>>   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.
>
> We're seeing a ton of fallout due to this, and given that there are already two different patterns that cause crashes, I'd like to have a known good version in the tree before we play whack-a-mole ;). Thus, I'd prefer that you reverted this and then recommitted with your proposed fix.

Hi, So as you probably saw, moments before this the change was pushed. The opinion in the other PR seemed to be its best to push the fix.
I don't see anything in the build logs this morning that looks related to the AtomicRMW code (seem to be blocked by something else though),
so my feeling is if things are fine with the 3rd fix, leave up, if we see ANY issue that may be related to the code, immediately reverting all
three is the way to go. Ping back here if you see any issue and I'll revert or you can just do so.

Sorry for the hassle this has caused :(

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939