[x86] Fix PR22524: the DAG combiner was incorrectly handling illegal

nodes when folding bitcasts of constants.

We can't fold things and then check after-the-fact whether it was legal.
Once we have formed the DAG node, arbitrary other nodes may have been
collapsed to it. There is no easy way to go back. Instead, we need to
test for the specific folding cases we're interested in and ensure those
are legal first.

This could in theory make this less powerful for bitcasting from an
integer to some vector type, but AFAICT, that can't actually happen in
the SDAG so its fine. Now, we *only* whitelist specific int->fp and
fp->int bitcasts for post-legalization folding. I've added the test case
from the PR.

(Also as a note, this does not appear to be in 3.6, no backport needed)

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@228656 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index 420883beafcc88bf003d4e3e5a3f7ac583e0b717..2c4f163d0b8a86e2c5776ce6c28efb3cbc12cea9 100644 (file)
--- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -6690,19 +6690,15 @@ SDValue DAGCombiner::visitBITCAST(SDNode *N) {
 
   // If the input is a constant, let getNode fold it.
   if (isa<ConstantSDNode>(N0) || isa<ConstantFPSDNode>(N0)) {
-    SDValue Res = DAG.getNode(ISD::BITCAST, SDLoc(N), VT, N0);
-    if (Res.getNode() != N) {
-      if (!LegalOperations ||
-          TLI.isOperationLegal(Res.getNode()->getOpcode(), VT))
-        return Res;
-
-      // Folding it resulted in an illegal node, and it's too late to
-      // do that. Clean up the old node and forego the transformation.
-      // Ideally this won't happen very often, because instcombine
-      // and the earlier dagcombine runs (where illegal nodes are
-      // permitted) should have folded most of them already.
-      deleteAndRecombine(Res.getNode());
-    }
+    // If we can't allow illegal operations, we need to check that this is just
+    // a fp -> int or int -> conversion and that the resulting operation will
+    // be legal.
+    if (!LegalOperations ||
+        (isa<ConstantSDNode>(N0) && VT.isFloatingPoint() && !VT.isVector() &&
+         TLI.isOperationLegal(ISD::ConstantFP, VT)) ||
+        (isa<ConstantFPSDNode>(N0) && VT.isInteger() && !VT.isVector() &&
+         TLI.isOperationLegal(ISD::Constant, VT)))
+      return DAG.getNode(ISD::BITCAST, SDLoc(N), VT, N0);
   }
 
   // (conv (conv x, t1), t2) -> (conv x, t2)

diff --git a/test/CodeGen/X86/constant-combines.ll b/test/CodeGen/X86/constant-combines.ll
new file mode 100644 (file)
index 0000000..d2a6ef4
--- /dev/null
+++ b/test/CodeGen/X86/constant-combines.ll
@@ -0,0 +1,35 @@
+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-unknown"
+
+define void @PR22524({ float, float }* %arg) {
+; Check that we can materialize the zero constants we store in two places here,
+; and at least form a legal store of the floating point value at the end.
+; The DAG combiner at one point contained bugs that given enough permutations
+; would incorrectly form an illegal operation for the last of these stores when
+; it folded it to a zero too late to legalize the zero store operation. If this
+; ever starts forming a zero store instead of movss, the test case has stopped
+; being useful.
+; 
+; CHECK-LABEL: PR22524:
+entry:
+  %0 = getelementptr inbounds { float, float }* %arg,  i32 0, i32 1
+  store float 0.000000e+00, float* %0, align 4
+; CHECK: movl $0, 4(%rdi)
+
+  %1 = getelementptr inbounds { float, float }* %arg, i64 0,  i32 0
+  %2 = bitcast float* %1 to i64*
+  %3 = load i64* %2, align 8
+  %4 = trunc i64 %3 to i32
+  %5 = lshr i64 %3, 32
+  %6 = trunc i64 %5 to i32
+  %7 = bitcast i32 %6 to float
+  %8 = fmul float %7, 0.000000e+00
+  %9 = bitcast float* %1 to i32*
+  store i32 %6, i32* %9, align 4
+; CHECK: movl $0, (%rdi)
+  store float %8, float* %0, align 4
+; CHECK: movss %{{.*}}, 4(%rdi)
+  ret void
+}