[PATCH] AArch64: Relax assert about large shift sizes.

[Ahmed Bougacha]

On Tue, Jan 13, 2015 at 2:42 AM, Matthias Braun <matze at braunis.de> wrote:
> Okay so all that constant folding logic is already there apparently. It just
> does not work properly in this case. The code in the combiner looks like
> this:
>
>   // fold (srl c1, c2) -> c1 >>u c2
>   if (N0C && N1C)
>     return DAG.FoldConstantArithmetic(ISD::SRL, VT, N0C, N1C);
>
> It enters this case as both operands are constants, however
> DAG.FoldConstantArithmetic() then bails out because one of the two constants
> has the Opaque flag set. Unfortunately the DAGCombiner does not try any
> other transformations after that, probably in the assumption that the
> constant folding will never fail there.
>
> I can see three possibilities to fix this, not sure which one would be best
> here (also the same situation exists in many other DAGCombiners which should
> be changed as well then):
>
> -  if (N0C && N1C && !N0C->isOpaque() && !N1C->isOpaque())
>       return DAG.FoldConstantArithmetic(ISD::SRL, VT, N0C, N1C);
> -  if (N0C && N1C) {
>      SDValue NewVal = DAG.FoldConstantArithmetic(ISD::SRL, VT, N0C, N1C);
>      if (NewVal.getNode())
>          return NewVal;
>    }
> - Allow constant folding with OpaqueConstants? I'm not completely sure what
> the semantic of an OpaqueConstant is, I guess it forces the constant to be
> materialized in a register intsead of becoming an immediate, but why would
> that forbid constant folding?

How about:
- try folding to undef before anything else (at least before constant-folding)?

> And BTW: The bitcast of a Constant becoming an OpaqueConstant seems to be
> intentional, as the ConstantHoisting pass appears to be relying on that from
> what I can see in the introduction comments there.
>
> Greetings
> Matthias
>
> On Jan 12, 2015, at 4:53 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
> wrote:
>
> On Tue, Jan 13, 2015 at 1:34 AM, Matthias Braun <matze at braunis.de> wrote:
>
> I think replacing it with undef would be legal, but isn't replacing this
> with undef a combine/constant fold thing? Should we duplicate that logic in
> the Selection DAG? I'm new to this part of llvm, so maybe I don't understand
> the philosophy here yet.
>
>
> The SelectionDAG has its own combiner (see
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp).
>
> A quick examination of visitSRL there shows the undef folding is after
> the both-constants folding, which I guess fails for some reason in
> this case (probably the bitcast).
>
> In any case, I think the change in your patch is OK; Quentin, what do you
> think?
>
> -Ahmed
>
> Greetings
> Matthias
>
> On Jan 12, 2015, at 4:14 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>
> Hi Matthias,
>
> I am wondering if we shouldn’t generate an undef node in Selection DAG for
> such shifts, unless the semantic is properly defined there.
>
> What do you think?
>
>
> On Jan 12, 2015, at 3:28 PM, Matthias Braun <matze at braunis.de> wrote:
>
> Shift sizes larger than the size of the value in bits can happen if
> constant folding fails.
>
> Testcase based on rdar://19211454
>
> http://reviews.llvm.org/D6940
>
> Files:
> lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
> test/CodeGen/AArch64/large_shift.ll
>
> Index: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
> ===================================================================
> --- lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
> +++ lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
> @@ -1424,8 +1424,10 @@
> } else
>   return false;
>
> -  assert((BiggerPattern || (Srl_imm > 0 && Srl_imm < VT.getSizeInBits()))
> &&
> -         "bad amount in shift node!");
> +  // Bail out on large immediates. This happens when no proper
> +  // combining/constant folding was performed.
> +  if (!BiggerPattern && (Srl_imm <= 0 || Srl_imm >= VT.getSizeInBits()))
> +    return false;
>
> LSB = Srl_imm;
> MSB = Srl_imm + (VT == MVT::i32 ? CountTrailingOnes_32(And_imm)
> Index: test/CodeGen/AArch64/large_shift.ll
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/AArch64/large_shift.ll
> @@ -0,0 +1,18 @@
> +; RUN: llc -march=aarch64 -o - %s
> +target triple = "arm64-unknown-unknown"
> +
> +declare void @t()
> +
> +define void @foo() {
> +  %c = bitcast i64 270458 to i64
> +  %t0 = lshr i64 %c, 422383
>
>
> What assembly is generated for this?
>
> Thanks,
> -Quentin
>
> +  %t1 = trunc i64 %t0 to i1
> +  br i1 %t1, label %BB1, label %BB0
> +
> +BB0:
> +  call void @t()
> +  br label %BB1
> +
> +BB1:
> +  ret void
> +}
>
> EMAIL PREFERENCES
> http://reviews.llvm.org/settings/panel/emailpreferences/
> <D6940.18054.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>