[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#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.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140939