[PATCH] D76319: [DAGCombiner] Fix non-determinism problem related to argument evaluation order in visitFDIV

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13052-13055
+    SDValue Neg0 =
+        TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize);
+    SDValue Neg1 =
+        TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize);
----------------
spatel wrote:
> bjope wrote:
> > bjope wrote:
> > > spatel wrote:
> > > > spatel wrote:
> > > > > If you reverse the order of these statements, does the gcc-built-LLVM still crash? Ie, is this a reliable/reproducible failure and fix?
> > > > Oops - just re-read the description text, and the answer is "yes".
> > > > Can you post the exact assert/unreachable that you are seeing?
> > > I didn't have the exact stack trace from the x86 reproducer at hand, but the below should give the picture.
> > > 
> > > And as you may have discovered I get the crash both when building llc using clang and gcc, as soon as I reverse the order here. So it should hopefully be easy to reproduce it (unless there is something even nastier involved).
> > > 
> > > ```
> > > Unknown code
> > > UNREACHABLE executed at ../lib/CodeGen/SelectionDAG/TargetLowering.cpp:5812!
> > > Stack dump:
> > >  #0 0x00000000027c7de5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/.../compiler-clang/bin/llc+0x27c7de5)
> > >  #1 0x00000000027c5a94 llvm::sys::RunSignalHandlers() (/.../compiler-clang/bin/llc+0x27c5a94)
> > >  #2 0x00000000027c5bd3 SignalHandler(int) (/.../compiler-clang/bin/llc+0x27c5bd3)
> > >  #3 0x00007fa8af489630 __restore_rt (/lib64/libpthread.so.0+0xf630)
> > >  #4 0x00007fa8ae224377 raise (/lib64/libc.so.6+0x36377)
> > >  #5 0x00007fa8ae225a68 abort (/lib64/libc.so.6+0x37a68)
> > >  #6 0x00000000027463fa (/.../compiler-clang/bin/llc+0x27463fa)
> > >  #7 0x000000000264fbcc llvm::TargetLowering::getNegatedExpression(llvm::SDValue, llvm::SelectionDAG&, bool, bool, unsigned int) const (/.../compiler-clang/bin/llc+0x264fbcc)
> > >  #8 0x000000000264f620 llvm::TargetLowering::getNegatedExpression(llvm::SDValue, llvm::SelectionDAG&, bool, bool, unsigned int) const (/.../compiler-clang/bin/llc+0x264f620)
> > >  #9 0x00000000025215c3 (anonymous namespace)::DAGCombiner::visitFDIV(llvm::SDNode*) (/.../compiler-clang/bin/llc+0x25215c3)
> > > #10 0x0000000002523ed1 (anonymous namespace)::DAGCombiner::visit(llvm::SDNode*) (/.../compiler-clang/bin/llc+0x2523ed1)
> > > #11 0x000000000252525c (anonymous namespace)::DAGCombiner::combine(llvm::SDNode*) (/.../compiler-clang/bin/llc+0x252525c)
> > > #12 0x0000000002526d8d (anonymous namespace)::DAGCombiner::Run(llvm::CombineLevel) (/.../compiler-clang/bin/llc+0x2526d8d)
> > > #13 0x0000000002529000 llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AAResults*, llvm::CodeGenOpt::Level) (/.../compiler-clang/bin/llc+0x2529000)
> > > #14 0x0000000002626330 llvm::SelectionDAGISel::CodeGenAndEmitDAG() (/.../compiler-clang/bin/llc+0x2626330)
> > > #15 0x000000000262ad58 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (/.../compiler-clang/bin/llc+0x262ad58)
> > > #16 0x000000000262c709 llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (.part.868) (/.../compiler-clang/bin/llc+0x262c709)
> > > #17 0x000000000096e709 (anonymous namespace)::MyDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) (/.../compiler-clang/bin/llc+0x96e709)
> > > #18 0x0000000001d1b01f llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/.../compiler-clang/bin/llc+0x1d1b01f)
> > > #19 0x00000000020e564e llvm::FPPassManager::runOnFunction(llvm::Function&) (/.../compiler-clang/bin/llc+0x20e564e)
> > > #20 0x00000000020e60f9 llvm::FPPassManager::runOnModule(llvm::Module&) (/.../compiler-clang/bin/llc+0x20e60f9)
> > > #21 0x00000000020e64c1 llvm::legacy::PassManagerImpl::run(llvm::Module&) (/.../compiler-clang/bin/llc+0x20e64c1)
> > > #22 0x00000000006bb219 main (/.../compiler-clang/bin/llc+0x6bb219)
> > > ```
> > > 
> > > 
> > > 
> > > 
> > So it is the "Unknown code" llvm_unreachable at the end of TargetLowering::getNegatedExpression that we hit.
> Thanks, so this bug exists independently of this change. Ie, I can get the crash when compiling LLVM with LLVM by commuting the fdiv operands in your test. Similarly, the bug should re-appear in the gcc-compiled binary after this fix by commuting the operands.
> 
> When we getNegatedExpression() on %sub4 in the test, it creates an extra use of %sub1. And that alters the subsequent getNegatedExpression() of %mul2.
> 
> So this patch is ok if you still want to make it, but we need to adjust the code and test comments. The underlying bug is still lurking.
Proposal for a better (but still incomplete) fix:
D76439

But I think we should go ahead with this patch too to reduce non-determinism.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76319