MachineSink: Fix strict weak ordering in GetAllSortedSuccessors

[arsenm]

Testcase?

[danlark]

In D155811#4520726, @arsenm wrote:

Testcase?

In libcxx debug mode at head (with https://reviews.llvm.org/D150264) we have pseudo_cmov_lower2.ll.test which fails before the patch and passes after that:

Stack dump:
0.  Program arguments: llc -mtriple=x86_64-linux-gnu -o -
1.  Running pass 'Function Pass Manager' on module '<stdin>'.
2.  Running pass 'Machine code sinking' on function '@foo5'
 #0 0x000055e9b3706d3e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (+0x6906d3e)
 #1 0x000055e9b370742c SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f2c887931c0 __restore_rt (/usr/grte/v5/lib64/libpthread.so.0+0x151c0)
 #3 0x00007f2c8863a347 gsignal (/usr/grte/v5/lib64/libc.so.6+0x75347)
 #4 0x00007f2c8863b797 abort (/usr/grte/v5/lib64/libc.so.6+0x76797)
 #5 0x000055e9b3a0e597 (+0x6c0e597)
 #6 0x000055e9b2496dc9 (anonymous namespace)::MachineSinking::FindSuccToSinkTo(llvm::MachineInstr&, llvm::MachineBasicBlock*, bool&, std::__u::map<llvm::MachineBasicBlock*, llvm::SmallVector<llvm::MachineBasicBlock*, 4u>, std::__u::less<llvm::MachineBasicBlock*>, std::__u::allocator<std::__u::pair<llvm::MachineBasicBlock* const, llvm::SmallVector<llvm::MachineBasicBlock*, 4u>>>>&) MachineSink.cpp:0:0
 #7 0x000055e9b2491d21 (anonymous namespace)::MachineSinking::runOnMachineFunction(llvm::MachineFunction&) MachineSink.cpp:0:0
 #8 0x000055e9b23e3fdc llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (+0x55e3fdc)
 #9 0x000055e9b34d43f4 llvm::FPPassManager::runOnFunction(llvm::Function&) (+0x66d43f4)
#10 0x000055e9b34d9ce2 llvm::FPPassManager::runOnModule(llvm::Module&) (+0x66d9ce2)
#11 0x000055e9b34d4a9d llvm::legacy::PassManagerImpl::run(llvm::Module&) (+0x66d4a9d)
#12 0x000055e9b0d75ef5 compileModule(char**, llvm::LLVMContext&) llc.cpp:0:0
#13 0x000055e9b0d737af main (+0x3f737af)
#14 0x00007f2c88626633 __libc_start_main (/usr/grte/v5/lib64/libc.so.6+0x61633)
#15 0x000055e9b0d7322a _start (+0x3f7322a)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  llvm/llvm-project/llvm/FileCheck --allow-unused-prefixes llvm/llvm-project/llvm/test/CodeGen/X86/pseudo_cmov_lower2.ll

[arsenm]

In D155811#4534689, @danlark wrote:

In D155811#4520726, @arsenm wrote:

Testcase?

In libcxx debug mode at head (with https://reviews.llvm.org/D150264) we have pseudo_cmov_lower2.ll.test which fails before the patch and passes after that:

Stack dump:
0.  Program arguments: llc -mtriple=x86_64-linux-gnu -o -
1.  Running pass 'Function Pass Manager' on module '<stdin>'.
2.  Running pass 'Machine code sinking' on function '@foo5'
 #0 0x000055e9b3706d3e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (+0x6906d3e)
 #1 0x000055e9b370742c SignalHandler(int) Signals.cpp:0:0
 #2 0x00007f2c887931c0 __restore_rt (/usr/grte/v5/lib64/libpthread.so.0+0x151c0)
 #3 0x00007f2c8863a347 gsignal (/usr/grte/v5/lib64/libc.so.6+0x75347)
 #4 0x00007f2c8863b797 abort (/usr/grte/v5/lib64/libc.so.6+0x76797)
 #5 0x000055e9b3a0e597 (+0x6c0e597)
 #6 0x000055e9b2496dc9 (anonymous namespace)::MachineSinking::FindSuccToSinkTo(llvm::MachineInstr&, llvm::MachineBasicBlock*, bool&, std::__u::map<llvm::MachineBasicBlock*, llvm::SmallVector<llvm::MachineBasicBlock*, 4u>, std::__u::less<llvm::MachineBasicBlock*>, std::__u::allocator<std::__u::pair<llvm::MachineBasicBlock* const, llvm::SmallVector<llvm::MachineBasicBlock*, 4u>>>>&) MachineSink.cpp:0:0
 #7 0x000055e9b2491d21 (anonymous namespace)::MachineSinking::runOnMachineFunction(llvm::MachineFunction&) MachineSink.cpp:0:0
 #8 0x000055e9b23e3fdc llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (+0x55e3fdc)
 #9 0x000055e9b34d43f4 llvm::FPPassManager::runOnFunction(llvm::Function&) (+0x66d43f4)
#10 0x000055e9b34d9ce2 llvm::FPPassManager::runOnModule(llvm::Module&) (+0x66d9ce2)
#11 0x000055e9b34d4a9d llvm::legacy::PassManagerImpl::run(llvm::Module&) (+0x66d4a9d)
#12 0x000055e9b0d75ef5 compileModule(char**, llvm::LLVMContext&) llc.cpp:0:0
#13 0x000055e9b0d737af main (+0x3f737af)
#14 0x00007f2c88626633 __libc_start_main (/usr/grte/v5/lib64/libc.so.6+0x61633)
#15 0x000055e9b0d7322a _start (+0x3f7322a)
FileCheck error: '<stdin>' is empty.
FileCheck command line:  llvm/llvm-project/llvm/FileCheck --allow-unused-prefixes llvm/llvm-project/llvm/test/CodeGen/X86/pseudo_cmov_lower2.ll

Something that incidentally crashed isn't the most satisfying test for something like this. A targeted, likely MIR test, would be better

[aeubanks]

this code presumably already has test coverage, so incidentally checking strict weak ordering in those tests via libc++ debug flags seems fine to me

[danlark]

In D155811#4554378, @aeubanks wrote:

this code presumably already has test coverage, so incidentally checking strict weak ordering in those tests via libc++ debug flags seems fine to me

I don't have commit rights. Please, submit for me. Danila Kutenin [e-mail]

[MaskRay]

I'll add this to the commit message when landing

CodeGen/X86/pseudo_cmov_lower2.ll fails using libc++ debug mode
(D150264) without this change.

llvm/lib/CodeGen/MachineSink.cpp

if (DTChild->getIDom()->getBlock() == MI.getParent() &&
       AllSuccs.push_back(DTChild->getBlock());
   }
 
   // Sort Successors according to their cycle depth or block frequency info.
   llvm::stable_sort(
       AllSuccs, [this](const MachineBasicBlock *L, const MachineBasicBlock *R) {
         uint64_t LHSFreq = MBFI ? MBFI->getBlockFreq(L).getFrequency() : 0;
         uint64_t RHSFreq = MBFI ? MBFI->getBlockFreq(R).getFrequency() : 0;
-        bool HasBlockFreq = LHSFreq != 0 && RHSFreq != 0;
+        bool HasBlockFreq = LHSFreq != 0 || RHSFreq != 0;
         return HasBlockFreq ? LHSFreq < RHSFreq
                             : CI->getCycleDepth(L) < CI->getCycleDepth(R);
       });
 
   auto it = AllSuccessors.insert(std::make_pair(MBB, AllSuccs));
 
   return it.first->second;
 }