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

[Matthias Braun]

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?

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 <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150112/ee62cc57/attachment.html>