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

[Alexander Kornienko via Phabricator via llvm-commits]

alexfh added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939