This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by goldstein.w.n on Jan 3 2023, 6:41 PM.

Details

Summary

This is essentially expanding on the optimizations added on: D120199
but applies the optimization to cases where the bit being changed /
tested is not am IMM but is a provable power of 2.

The only case currently added for cases like:
__atomic_fetch_xor(p, 1 << c, __ATOMIC_RELAXED) & (1 << c)

Which instead of using a cmpxchg loop can be done with btcl; setcc; shl.

There are still a variety of missed cases that could/should be
addressed in the future. This commit documents many of those
cases with Todos.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Jan 3 2023, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 6:41 PM
goldstein.w.n requested review of this revision.Jan 3 2023, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 6:41 PM
goldstein.w.n added a comment.EditedJan 3 2023, 6:43 PM

This is patch [2/2].

Note: This is a follow up to https://reviews.llvm.org/D140645 and is splitting up the reviews for the tests and code.

Propegate BMI test removal

craig.topper retitled this revision from Transform AtomicRMW logic operations to BT{R|C|S} if only changing/testing a single bit. to [X86] Transform AtomicRMW logic operations to BT{R|C|S} if only changing/testing a single bit..Jan 4 2023, 4:49 PM
pengfei added inline comments.Jan 5 2023, 5:58 AM
llvm/lib/Target/X86/X86ISelLowering.cpp
5653

Should the Size be same with operand 1 rather than result?

31434

or

31435

No parentheses needed for single line scope.

31453

Should it be a canonical form that we can assume it is always X ^ -1?

31454

We can use match(....) for better readability?

31527

Is enough to only check BitChange.second == UndefBit?

31559

Missing the last period .

31564

ditto.

llvm/test/CodeGen/X86/atomic-rm-bit-test.ll
8081–8087

The branch code doesn't look necessary. Can we necessary it?

goldstein.w.n marked 6 inline comments as done.

Use match instead of hand written logic.
Fix some typos.
Fix some code style nits.

llvm/lib/Target/X86/X86ISelLowering.cpp
5653

Should the Size be same with operand 1 rather than result?

If I change it I run into assertion errors.

I.getArgOperand(0)->getType()->getScalarSizeInBits() the Size can be zero (bitwidth too small)

or I.getArgOperand(1)->getType()->getScalarSizeInBits() I run into:

llc: /home/noah/programs/opensource/llvm-dev/src/llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:11108: llvm::MemSDNode::MemSDNode(unsigned int, unsigned int, const llvm::DebugLoc&, llvm::SDVTList, llvm::EVT, llvm::MachineMemOperand*): Assertion `memvt.getStoreSize().getKnownMinSize() <= MMO->getSize() && "Size mismatch!"' failed.

Maybe that means there is a bug in the patch elsewhere?

Running the test here are the sizes I see:

Sizes: 8 / 0 / 16
Sizes: 8 / 0 / 16
Sizes: 8 / 0 / 16
Sizes: 8 / 0 / 16
Sizes: 16 / 0 / 8
Sizes: 16 / 0 / 8
Sizes: 16 / 0 / 8
Sizes: 16 / 0 / 8
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 8 / 0 / 32
Sizes: 32 / 0 / 8
Sizes: 32 / 0 / 8
Sizes: 32 / 0 / 8
Sizes: 32 / 0 / 8
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 8 / 0 / 64
Sizes: 64 / 0 / 8
Sizes: 64 / 0 / 8
Sizes: 64 / 0 / 8
31453

Should it be a canonical form that we can assume it is always X ^ -1?

Probably defacto fixed by using match (didn't know match existed when I wrote this patch :/ alot to learn).

llvm/test/CodeGen/X86/atomic-rm-bit-test.ll
8081–8087

The branch code doesn't look necessary. Can we necessary it?

I think it is b.c we don't cmovcc loads.

For

if.then:                                          ; preds = %entry
  %idxprom = zext i32 %c to i64
  %arrayidx = getelementptr inbounds i32, ptr %v, i64 %idxprom
  %1 = load i32, ptr %arrayidx, align 4
  br label %return

And

return:                                           ; preds = %entry, %if.then
  %retval.0 = phi i32 [ %1, %if.then ], [ 123, %entry ]
  ret i32 %retval.0

a branch seems correct.

lebedev.ri edited the summary of this revision. (Show Details)Jan 5 2023, 9:38 AM
pengfei added inline comments.Jan 5 2023, 6:36 PM
llvm/test/CodeGen/X86/atomic-rm-bit-test.ll
8081–8087

Sorry for the wrong words. I mean can we eliminate the branch by modifying the IR code, e.g.,

entry:
  %shl = shl nuw i32 1, %c
  %0 = atomicrmw or ptr %v, i32 %shl monotonic, align 4
  %and = and i32 %0, %shl
  %tobool.not = icmp eq i32 %and, 0
  %ret = zext i1 %tobool.not to i32
  ret i32 %ret

This will help to reduce the nosie in reviewing the code and pay more attention to the change we expected.

pengfei added inline comments.Jan 5 2023, 6:39 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5653

I mean split x86_atomic_bt*_rm from it and then change it to I.getArgOperand(1)->getType()->getScalarSizeInBits()
Other intrinsics are correct to use I.getType()->getScalarSizeInBits().

goldstein.w.n added inline comments.Jan 5 2023, 10:12 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5653

I mean split x86_atomic_bt*_rm from it and then change it to I.getArgOperand(1)->getType()->getScalarSizeInBits()
Other intrinsics are correct to use I.getType()->getScalarSizeInBits().

What does "split x86_atomic_bt*_rm` from it" mean?

llvm/test/CodeGen/X86/atomic-rm-bit-test.ll
8081–8087

Sorry for the wrong words. I mean can we eliminate the branch by modifying the IR code, e.g.,

entry:
  %shl = shl nuw i32 1, %c
  %0 = atomicrmw or ptr %v, i32 %shl monotonic, align 4
  %and = and i32 %0, %shl
  %tobool.not = icmp eq i32 %and, 0
  %ret = zext i1 %tobool.not to i32
  ret i32 %ret

This will help to reduce the nosie in reviewing the code and pay more attention to the change we expected.

I see, so there are 6 difference test types:
_br -> branch on value
_brz -> branch on !value
_brnz -> branch on !!value
_val -> return value
_valz -> return !value
_valnz -> return !!value

Imo they are all worth testing. For example we have logic that searches the uses to see if its only for a truth value to see if we can optimize out the shift.
IIRC when writing the code, there where some edge cases where br behaved differently than setc so think its worth keeping.

pengfei added inline comments.Jan 5 2023, 10:15 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
5653
case Intrinsic::x86_cmpccxadd32:
case Intrinsic::x86_cmpccxadd64:
case Intrinsic::x86_atomic_bts:
case Intrinsic::x86_atomic_btc:
case Intrinsic::x86_atomic_btr: {
  ... ...
}
case Intrinsic::x86_atomic_bts_rm:
case Intrinsic::x86_atomic_btc_rm:
case Intrinsic::x86_atomic_btr_rm: {
  ... ...
}

Propegate test changes.
Use proper size

goldstein.w.n marked an inline comment as done.Jan 5 2023, 11:18 PM
goldstein.w.n added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
5653
case Intrinsic::x86_cmpccxadd32:
case Intrinsic::x86_cmpccxadd64:
case Intrinsic::x86_atomic_bts:
case Intrinsic::x86_atomic_btc:
case Intrinsic::x86_atomic_btr: {
  ... ...
}
case Intrinsic::x86_atomic_bts_rm:
case Intrinsic::x86_atomic_btc_rm:
case Intrinsic::x86_atomic_btr_rm: {
  ... ...
}

Done, for some reason had thought they where in different cases.

goldstein.w.n marked an inline comment as done.Jan 12 2023, 11:33 AM

ping.

pengfei accepted this revision.Jan 13 2023, 4:57 AM

LGTM.

This revision is now accepted and ready to land.Jan 13 2023, 4:57 AM
This revision was landed with ongoing or failed builds.Jan 16 2023, 10:09 PM
This revision was automatically updated to reflect the committed changes.

Hey, it looks like this change triggered an assertion failure in the Fuchsia continous build: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8791734713064712625/overview

clang: llvm/lib/Target/X86/X86ISelLowering.cpp:31525: TargetLowering::AtomicExpansionKind llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR(AtomicRMWInst *) const: Assertion `I->getOperand(0)...
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: ../../../recipe_cleanup/clangs6i4kjx7/bin/clang -MD -MF obj/src/connectivity/wlan/drivers/third_party/intel/iwlwifi/mvm/mvm.sta.c.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS ...
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module '../../src/connectivity/wlan/drivers/third_party/intel/iw...

Hey, it looks like this change triggered an assertion failure in the Fuchsia continous build: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8791734713064712625/overview

clang: llvm/lib/Target/X86/X86ISelLowering.cpp:31525: TargetLowering::AtomicExpansionKind llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR(AtomicRMWInst *) const: Assertion `I->getOperand(0)...
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: ../../../recipe_cleanup/clangs6i4kjx7/bin/clang -MD -MF obj/src/connectivity/wlan/drivers/third_party/intel/iwlwifi/mvm/mvm.sta.c.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS ...
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module '../../src/connectivity/wlan/drivers/third_party/intel/iw...

The easiest thing to do now would be:

-  assert(I->getOperand(0) == AI);
+  if (I->getOperand(0) != AI)
+    return AtomicExpansionKind::CmpXChg;

or revert.

Although I think if that assertion is being hit the old code was buggy:

// The following instruction must be a AND single bit.
auto *C2 = dyn_cast<ConstantInt>(I->getOperand(1));
unsigned Bits = AI->getType()->getPrimitiveSizeInBits();
if (!C2 || Bits == 8 || !isPowerOf2_64(C2->getZExtValue()))
  return AtomicExpansionKind::CmpXChg;

if (AI->getOperation() == AtomicRMWInst::And)
  return ~C1->getValue() == C2->getValue()
             ? AtomicExpansionKind::BitTestIntrinsic
             : AtomicExpansionKind::CmpXChg;

return C1 == C2 ? AtomicExpansionKind::BitTestIntrinsic
                : AtomicExpansionKind::CmpXChg;

Was assuming the above assert AFAICT.

goldstein.w.n added a comment.EditedJan 19 2023, 4:54 PM

Hey, it looks like this change triggered an assertion failure in the Fuchsia continous build: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8791734713064712625/overview

clang: llvm/lib/Target/X86/X86ISelLowering.cpp:31525: TargetLowering::AtomicExpansionKind llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR(AtomicRMWInst *) const: Assertion `I->getOperand(0)...
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: ../../../recipe_cleanup/clangs6i4kjx7/bin/clang -MD -MF obj/src/connectivity/wlan/drivers/third_party/intel/iwlwifi/mvm/mvm.sta.c.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS ...
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module '../../src/connectivity/wlan/drivers/third_party/intel/iw...

Created: https://reviews.llvm.org/D142166 Its a proper fix and tested. Otherwise revert is probably best for now

Hey, it looks like this change triggered an assertion failure in the Fuchsia continous build: https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8791734713064712625/overview

clang: llvm/lib/Target/X86/X86ISelLowering.cpp:31525: TargetLowering::AtomicExpansionKind llvm::X86TargetLowering::shouldExpandLogicAtomicRMWInIR(AtomicRMWInst *) const: Assertion `I->getOperand(0)...
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: ../../../recipe_cleanup/clangs6i4kjx7/bin/clang -MD -MF obj/src/connectivity/wlan/drivers/third_party/intel/iwlwifi/mvm/mvm.sta.c.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS ...
1.	<eof> parser at end of file
2.	Code generation
3.	Running pass 'Function Pass Manager' on module '../../src/connectivity/wlan/drivers/third_party/intel/iw...

Created: https://reviews.llvm.org/D142166 Its a proper fix and tested. Otherwise revert is probably best for now

D142166 has been accepted with some nits. Going to wait ~24hours to any remaining comments, then push.

alexfh added a subscriber: alexfh.Jan 22 2023, 11:46 PM

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

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

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()
goldstein.w.n added a comment.EditedJan 23 2023, 1:53 AM

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.

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.

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.

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.

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.

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.

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 :(

n-omer added a subscriber: n-omer.Mar 12 2023, 3:41 AM