[llvm-commits] [PATCH] ARM: Custom lower v2f32 = fp_round to fix crash with NEON

[Pete Couperus]

Hi David,

Thanks again for taking a look.  I've modified the patch to address your
points.  You're right, the docs indicate that Fixed Size Arrays are
preferred over SmallVectors in this case.  Seems that both have been used
in similar cases.
Thanks!

Pete


On Wed, Nov 14, 2012 at 5:47 PM, David Peixotto <dpeixott at codeaurora.org>wrote:

> Hi Pete,****
>
> ** **
>
> Thanks for fixing this bug! I have a few minor comments on the patch below.
> ****
>
> ****
>
> Index: lib/Target/ARM/ARMISelLowering.cpp****
>
> ===================================================================****
>
> --- lib/Target/ARM/ARMISelLowering.cpp            (revision 167913)****
>
> +++ lib/Target/ARM/ARMISelLowering.cpp         (working copy)****
>
> ****
>
> +SDValue ARMTargetLowering::LowerFP_ROUND(SDValue Op, SelectionDAG &DAG)
> const {****
>
> +  assert(Op.getValueType() == MVT::v2f32 &&****
>
> +         "FP_ROUND custom lowering should only occur for v2f32.");****
>
> +****
>
> +  DebugLoc DL = Op.getDebugLoc();****
>
> +  SmallVector<SDValue, 2> Vals;****
>
> ** **
>
> I think a fixed size array would work fine here. It is preferred over
> SmallVector in the llvm programmers manual:
> http://llvm.org/docs/ProgrammersManual.html#ds_sequential****
>
> ** **
>
> +  for(unsigned i = 0; i < 2; i++) {****
>
> +    SDValue Val = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, MVT::f64,****
>
> +                              Op.getOperand(0), DAG.getConstant(i,
> MVT::i32));****
>
> +    Vals.push_back(DAG.getNode(ISD::FP_ROUND, DL, MVT::f32,****
>
> +                               Val, DAG.getIntPtrConstant(0)));****
>
> +  }****
>
> +  EVT VecVT = EVT::getVectorVT(*DAG.getContext(), MVT::f32, 2);****
>
> +  return DAG.getNode(ISD::BUILD_VECTOR, DL, VecVT, &Vals[0], 2);****
>
> +}****
>
> +****
>
> ** **
>
> Index: test/CodeGen/ARM/neon_fpconv.ll****
>
> ===================================================================****
>
> --- test/CodeGen/ARM/neon_fpconv.ll                (revision 0)****
>
> +++ test/CodeGen/ARM/neon_fpconv.ll             (revision 0)****
>
> @@ -0,0 +1,9 @@****
>
> +; RUN: llc < %s -march=arm -mattr=+neon | FileCheck %s****
>
> ** **
>
> Please add a comment that this test is for PR12540.****
>
> ** **
>
> +****
>
> +define <2 x float> @vtrunc(<2 x double> %a) {****
>
> +; CHECK: vcvt.f32.f64 [[S0:s[0-9]+]], [[D0:d[0-9]+]]****
>
> +; CHECK: vcvt.f32.f64 [[S1:s[0-9]+]], [[D1:d[0-9]+]]****
>
> +  %vt = fptrunc <2 x double> %a to <2 x float>****
>
> +  ret <2 x float> %vt****
>
> +}****
>
> +****
>
> ** **
>
> Thanks,****
>
> David****
>
> ** **
>
> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation****
>
> ** **
>
> ** **
>
> *From:* llvm-commits-bounces at cs.uiuc.edu [mailto:
> llvm-commits-bounces at cs.uiuc.edu] *On Behalf Of *Pete Couperus
> *Sent:* Tuesday, November 13, 2012 11:24 PM
> *To:* llvm-commits at cs.uiuc.edu
> *Subject:* [llvm-commits] [PATCH] ARM: Custom lower v2f32 = fp_round to
> fix crash with NEON****
>
> ** **
>
> Hello,
>
> Lowering "fptrunc <2 x double> %a to <2 x float>" is broken on ARM with
> NEON.
> This patch custom lowers this conversion using two single element vcvt's.
> This fixes the following PRs.
>
> http://llvm.org/bugs/show_bug.cgi?id=12540
> http://llvm.org/bugs/show_bug.cgi?id=13964
>
> Please review!
> Thanks.
>
> Pete****
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121114/b352b58b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr12540.diff
Type: application/octet-stream
Size: 3334 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121114/b352b58b/attachment.obj>