[llvm] d168b77 - [DAGCombiner] Fix non-determinism problem related to argument evaluation order in visitFDIV

[Bjorn Pettersson via llvm-commits]

Author: Bjorn Pettersson
Date: 2020-03-20T16:11:17+01:00
New Revision: d168b7778035af6cc795b2367ca7f379ce1a629e

URL: https://github.com/llvm/llvm-project/commit/d168b7778035af6cc795b2367ca7f379ce1a629e
DIFF: https://github.com/llvm/llvm-project/commit/d168b7778035af6cc795b2367ca7f379ce1a629e.diff

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

Summary:
For some reason the order in which we call getNegatedExpression
for the involved operands, after a call to isCheaperToUseNegatedFPOps,
seem to matter. This patch includes a new test case in
test/CodeGen/X86/fdiv.ll that crashes if we reverse the order of
those calls. Before this patch that could happen depending on
which compiler that were used when buildind llvm. With my GCC
version (7.4.0) I got the crash, because it seems like it is
using a different order for the argument evaluation compared
to clang.

All other users of isCheaperToUseNegatedFPOps already used this
pattern with unfolded/ordered calls to getNegatedExpression, so
this patch is aligning visitFDIV with the other use cases.

This patch simply deals with the non-determinism for FDIV. While
the underlying problem with getNegatedExpression is discussed
further in D76439.

Reviewers: spatel, RKSimon

Reviewed By: spatel

Subscribers: hiraditya, mgrang, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D76319

Added: 
    

Modified: 
    llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
    llvm/test/CodeGen/X86/fdiv.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 254669b36470..f88950fc872a 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -13054,11 +13054,13 @@ SDValue DAGCombiner::visitFDIV(SDNode *N) {
   }
 
   // (fdiv (fneg X), (fneg Y)) -> (fdiv X, Y)
-  if (isCheaperToUseNegatedFPOps(N0, N1))
-    return DAG.getNode(
-        ISD::FDIV, SDLoc(N), VT,
-        TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize),
-        TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize), Flags);
+  if (isCheaperToUseNegatedFPOps(N0, N1)) {
+    SDValue Neg0 =
+        TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize);
+    SDValue Neg1 =
+        TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize);
+    return DAG.getNode(ISD::FDIV, SDLoc(N), VT, Neg0, Neg1, Flags);
+  }
 
   return SDValue();
 }

diff  --git a/llvm/test/CodeGen/X86/fdiv.ll b/llvm/test/CodeGen/X86/fdiv.ll
index 259cd91cca52..c361ab0f5aed 100644
--- a/llvm/test/CodeGen/X86/fdiv.ll
+++ b/llvm/test/CodeGen/X86/fdiv.ll
@@ -76,5 +76,29 @@ define <4 x float> @double_negative_vector(<4 x float> %x, <4 x float> %y) #0 {
   ret <4 x float> %div
 }
 
+; This test used to fail, depending on how llc was built (e.g. using
+; clang/gcc), due to order of argument evaluation not being well defined. We
+; ended up hitting llvm_unreachable in getNegatedExpression when building with
+; gcc. Just make sure that we get a deterministic result.
+define float @fdiv_fneg_combine(float %a0, float %a1, float %a2) #0 {
+; CHECK-LABEL: fdiv_fneg_combine:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movaps %xmm0, %xmm3
+; CHECK-NEXT:    subss %xmm1, %xmm3
+; CHECK-NEXT:    subss %xmm0, %xmm1
+; CHECK-NEXT:    mulss %xmm2, %xmm1
+; CHECK-NEXT:    subss %xmm2, %xmm3
+; CHECK-NEXT:    divss %xmm3, %xmm1
+; CHECK-NEXT:    movaps %xmm1, %xmm0
+; CHECK-NEXT:    retq
+  %sub1 = fsub fast float %a0, %a1
+  %mul2 = fmul fast float %sub1, %a2
+  %neg = fneg fast float %a0
+  %add3 = fadd fast float %a1, %neg
+  %sub4 = fadd fast float %add3, %a2
+  %div5 = fdiv fast float %mul2, %sub4
+  ret float %div5
+}
+
 attributes #0 = { "unsafe-fp-math"="false" }