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

Mon Feb 9 19:00:33 PST 2015

Thanks for the quick fix!

> On 2015-Feb-09, at 18:25, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> 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)

@Hans: I think Chandler was relying on my bisection from the PR here, but
I had only bisected the opt+llc.ll case.  (I first tried to bisect the
llc.ll case, but couldn't find a passing revision.)

I've just checked the llc.ll case on 3.6 (well, slightly old r228165) and
it *does* repro, and cherry-picking this commit fixes it.

Probably not *urgent*, since -instcombine isn't exposing it, but still
seems like a good fix.

@Chandler, I assume you're fine with this going in?

> 
> 
> 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
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits