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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added reviewers: spatel, RKSimon.
Herald added subscribers: mgrang, hiraditya.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76319

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


Index: llvm/test/CodeGen/X86/fdiv.ll
===================================================================
--- llvm/test/CodeGen/X86/fdiv.ll
+++ llvm/test/CodeGen/X86/fdiv.ll
@@ -76,5 +76,28 @@
   ret <4 x float> %div
 }
 
+; This used to crash, 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.
+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" }
 
Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -13043,11 +13043,19 @@
   }
 
   // (fdiv (fneg X), (fneg Y)) -> (fdiv X, Y)
-  if (isCheaperToUseNegatedFPOps(N0, N1))
+  if (isCheaperToUseNegatedFPOps(N0, N1)) {
+    // Notice that the order of these getNegatedExpression calls seem to
+    // matter. There is a test case in test/CodeGen/X86/fdiv.ll that crashed
+    // when doing the calls in reversed order (e.g. by letting the order depend
+    // on argument evaluation order by folding the calls inside the getNode()
+    // call).
+    SDValue Neg0 =
+        TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize);
+    SDValue Neg1 =
+        TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize);
     return DAG.getNode(
-        ISD::FDIV, SDLoc(N), VT,
-        TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize),
-        TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize), Flags);
+        ISD::FDIV, SDLoc(N), VT, Neg0, Neg1, Flags);
+  }
 
   return SDValue();
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76319.250892.patch
Type: text/x-patch
Size: 2360 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200317/cd5379d1/attachment.bin>