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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939