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

[Chandler Carruth]

Author: chandlerc
Date: Mon Feb  9 20:25:56 2015
New Revision: 228656

URL: http://llvm.org/viewvc/llvm-project?rev=228656&view=rev
Log:
[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)

Added:
    llvm/trunk/test/CodeGen/X86/constant-combines.ll
Modified:
    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=228656&r1=228655&r2=228656&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
+++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Mon Feb  9 20:25:56 2015
@@ -6690,19 +6690,15 @@ SDValue DAGCombiner::visitBITCAST(SDNode
 
   // 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)

Added: llvm/trunk/test/CodeGen/X86/constant-combines.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/constant-combines.ll?rev=228656&view=auto
==============================================================================
--- llvm/trunk/test/CodeGen/X86/constant-combines.ll (added)
+++ llvm/trunk/test/CodeGen/X86/constant-combines.ll Mon Feb  9 20:25:56 2015
@@ -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
+}