[llvm] r278002 - [AArch64] PR28877: Don't assume we're running after legalization when creating vcvtfp2fxs

Tue Aug 9 14:26:34 PDT 2016

The problem on the PR does reproduce on 3.9.

Since someone ran into this with real code, perhaps we should consider
to merge it?

Tim, Renato: what do you think?

On Tue, Aug 9, 2016 at 2:19 PM, Silviu Baranga <Silviu.Baranga at arm.com> wrote:
> As far as can see this was a latent bug from around late 2015, but the PR
> was reported for trunk.
>
> We could attempt running the regression test on 3.9, and if the bug is there
> it should trigger an assert.
>
>
> -Silviu
>
> ________________________________
> From: hwennborg at google.com <hwennborg at google.com> on behalf of Hans Wennborg
> <hans at chromium.org>
> Sent: 09 August 2016 21:43:02
> To: Silviu Baranga
> Cc: llvm-commits
> Subject: Re: [llvm] r278002 - [AArch64] PR28877: Don't assume we're running
> after legalization when creating vcvtfp2fxs
>
> Was this a bug affecting 3.9, or a more recent regression?
>
> On Mon, Aug 8, 2016 at 6:13 AM, Silviu Baranga via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>> Author: sbaranga
>> Date: Mon Aug  8 08:13:57 2016
>> New Revision: 278002
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=278002&view=rev
>> Log:
>> [AArch64] PR28877: Don't assume we're running after legalization when
>> creating vcvtfp2fxs
>>
>> Summary:
>> The DAG combine transformation that was generating the
>> aarch64_neon_vcvtfp2fxs node was assuming that all
>> inputs where legal and wasn't accounting that the input
>> could be a v4f64 if we're trying to do the transformation
>> before legalization. We now bail out in this case.
>>
>> All illegal types besides v4f64 were already rejected.
>>
>> Fixes https://llvm.org/bugs/show_bug.cgi?id=28877.
>>
>> Reviewers: jmolloy
>>
>> Subscribers: aemerson, rengolin, llvm-commits
>>
>> Differential Revision: https://reviews.llvm.org/D23261
>>
>> Added:
>>     llvm/trunk/test/CodeGen/AArch64/aarch64-vcvtfp2fxs-combine.ll
>> Modified:
>>     llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>>
>> Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=278002&r1=278001&r2=278002&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp (original)
>> +++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp Mon Aug  8
>> 08:13:57 2016
>> @@ -7684,6 +7684,7 @@ static SDValue performIntToFpCombine(SDN
>>  /// Fold a floating-point multiply by power of two into floating-point to
>>  /// fixed-point conversion.
>>  static SDValue performFpToIntCombine(SDNode *N, SelectionDAG &DAG,
>> +                                     TargetLowering::DAGCombinerInfo
>> &DCI,
>>                                       const AArch64Subtarget *Subtarget) {
>>    if (!Subtarget->hasNEON())
>>      return SDValue();
>> @@ -7727,10 +7728,16 @@ static SDValue performFpToIntCombine(SDN
>>      ResTy = FloatBits == 32 ? MVT::v2i32 : MVT::v2i64;
>>      break;
>>    case 4:
>> -    ResTy = MVT::v4i32;
>> +    ResTy = FloatBits == 32 ? MVT::v4i32 : MVT::v4i64;
>>      break;
>>    }
>>
>> +  if (ResTy == MVT::v4i64 && DCI.isBeforeLegalizeOps())
>> +    return SDValue();
>> +
>> +  assert((ResTy != MVT::v4i64 || DCI.isBeforeLegalizeOps()) &&
>> +         "Illegal vector type after legalization");
>> +
>>    SDLoc DL(N);
>>    bool IsSigned = N->getOpcode() == ISD::FP_TO_SINT;
>>    unsigned IntrinsicOpcode = IsSigned ?
>> Intrinsic::aarch64_neon_vcvtfp2fxs
>> @@ -9852,7 +9859,7 @@ SDValue AArch64TargetLowering::PerformDA
>>      return performIntToFpCombine(N, DAG, Subtarget);
>>    case ISD::FP_TO_SINT:
>>    case ISD::FP_TO_UINT:
>> -    return performFpToIntCombine(N, DAG, Subtarget);
>> +    return performFpToIntCombine(N, DAG, DCI, Subtarget);
>>    case ISD::FDIV:
>>      return performFDivCombine(N, DAG, Subtarget);
>>    case ISD::OR:
>>
>> Added: llvm/trunk/test/CodeGen/AArch64/aarch64-vcvtfp2fxs-combine.ll
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/aarch64-vcvtfp2fxs-combine.ll?rev=278002&view=auto
>>
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/AArch64/aarch64-vcvtfp2fxs-combine.ll (added)
>> +++ llvm/trunk/test/CodeGen/AArch64/aarch64-vcvtfp2fxs-combine.ll Mon Aug
>> 8 08:13:57 2016
>> @@ -0,0 +1,24 @@
>> +; RUN: llc < %s -mtriple=aarch64-linux-eabi -o - | FileCheck %s
>> +
>> +%struct.a= type { i64, i64, i64, i64 }
>> +
>> +; DAG combine will try to perform a transformation that  creates a
>> vcvtfp2fxs
>> +; with a v4f64 input. Since v4i64 is not legal we should bail out. We can
>> +; pottentially still create the vcvtfp2fxs node after legalization (but
>> on a
>> +; v2f64).
>> +
>> +; CHECK-LABEL: fun1
>> +define void @fun1() local_unnamed_addr {
>> +entry:
>> +  %mul = fmul <4 x double> zeroinitializer, <double 6.553600e+04, double
>> 6.553600e+04, double 6.553600e+04, double 6.553600e+04>
>> +  %toi = fptosi <4 x double> %mul to <4 x i64>
>> +  %ptr = getelementptr inbounds %struct.a, %struct.a* undef, i64 0, i32 2
>> +  %elem = extractelement <4 x i64> %toi, i32 1
>> +  store i64 %elem, i64* %ptr, align 8
>> +  call void @llvm.trap()
>> +  unreachable
>> +}
>> +
>> +; Function Attrs: noreturn nounwind
>> +declare void @llvm.trap()
>> +
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>