From patchwork Sun Feb 8 02:24:37 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Pinski X-Patchwork-Id: 437663 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 96F1414016A for ; Sun, 8 Feb 2015 13:25:24 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=ieY4aayu+xXJX/iREk IrgwNRslSLuUYQBF6oDT+c4GaFkn/N6kcUM7eDiOx085Emk8/r2S9URGnO0dJlRP XvoQ8CeoljcqvOmf9lL+GyPXXCY5lgUcWjeCbhw0YPkiONMz+Dv5WSrafvBH43oC XymssJ4pgP3Pavm7VHjqchfZA= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=gxi/Iv22ygBthTaa9RAgdD5D wxw=; b=AFalixRJ3/6Lp3NIlu/V0uNqqLi9HSsfQnyxw37MuE/1YQKXjEp6MXDd j5zRLeAr3seIIYCVPieb4F+Vn8N3IQHCQCoX2eXgjDq2o+0Q4burxyPtqVgrWU7F IgRkVQa4iCOxhuOV0OFpX6oPUrsUQS8ZIPklaceWj0XC9ar4j5E= Received: (qmail 30351 invoked by alias); 8 Feb 2015 02:24:48 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 30301 invoked by uid 89); 8 Feb 2015 02:24:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-lb0-f172.google.com Received: from mail-lb0-f172.google.com (HELO mail-lb0-f172.google.com) (209.85.217.172) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Sun, 08 Feb 2015 02:24:41 +0000 Received: by mail-lb0-f172.google.com with SMTP id l4so24227865lbv.3 for ; Sat, 07 Feb 2015 18:24:37 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.152.28.227 with SMTP id e3mr9803291lah.117.1423362277491; Sat, 07 Feb 2015 18:24:37 -0800 (PST) Received: by 10.25.21.3 with HTTP; Sat, 7 Feb 2015 18:24:37 -0800 (PST) In-Reply-To: References: <20150203073744.GV1746@tucnak.redhat.com> Date: Sat, 7 Feb 2015 18:24:37 -0800 Message-ID: Subject: Re: [PATCH/AARCH64] Fix 64893: ICE with vget_lane_u32 with C++ front-end at -O0 From: Andrew Pinski To: Jakub Jelinek Cc: GCC Patches , Alan Lawrence X-IsSubscribed: yes On Fri, Feb 6, 2015 at 5:02 PM, Andrew Pinski wrote: > On Mon, Feb 2, 2015 at 11:37 PM, Jakub Jelinek wrote: >> On Mon, Feb 02, 2015 at 02:51:43PM -0800, Andrew Pinski wrote: >>> While trying to build the GCC 5 with GCC 5, I ran into an ICE when >>> building libcpp at -O0. The problem is the C++ front-end was not >>> folding sizeof(a)/sizeof(a[0]) when passed to a function at -O0. The >>> C++ front-end keeps around sizeof until the gimplifier and there is no >>> way to fold the expressions that involve them. So to work around the >>> issue we need to change __builtin_aarch64_im_lane_boundsi to accept an >>> extra argument and change the first two arguments to size_t type so we >>> don't get an extra cast there and do the division inside the compiler >>> itself. >> >> Relying on anything being folded at -O0 when the language does not guarantee >> it is going to be more and more of a problem. So I think your patch is >> reasonable (of course, I'll defer this to target maintainers). >> >>> + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); >>> + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); >>> + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize)) >>> + { >>> + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); >>> + if (CONST_INT_P (lane_idx)) >>> + aarch64_simd_lane_bounds (lane_idx, 0, UINTVAL (totalsize)/UINTVAL (elementsize), exp); >> >> Too long line? Also, missing spaces around / . And, ICE if >> somebody uses __builtin_aarch64_im_lane_boundsi (4, 0, 0); >> So you need to check and complain for zero elementsize too. > > Good points, I don't know why I missed this. > >> >>> + else >>> + error ("%Klane index must be a constant immediate", exp); >>> + } >>> else >>> - error ("%Klane index must be a constant immediate", exp); >>> + sorry ("%Ktotal size and element size must be a constant immediate", exp); >> >> But why sorry? If you say the builtin requires constant arguments, then it >> is not sorry, but error, it is not an unimplemented feature. > > Because I originally thought that would be better than error but now > thinking this over, I was incorrect, it should be an error. > I will add a testcase for the __builtin_aarch64_im_lane_boundsi (4, 0, > 0) case and retest the patch. Here is the updated patch with Jakub's comments included and added a testcase for the 0, 0 case. Thanks, Andrew Pinski ChangeLog: PR target/64893 * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): Change the first argument type to size_type_node and add another size_type_node. (aarch64_simd_expand_builtin): Handle the new argument to AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather print an out when the first two arguments are not nonzero integer constants. * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. testsuite/ChangeLog: * c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. * c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. > > Thanks, > Andrew > > >> >> Jakub commit b5c809bddf8a34f490ee0cd540f12be1b8b2b897 Author: Andrew Pinski Date: Mon Feb 2 18:40:08 2015 +0000 Fix bug 64893: ICE with vget_lane_u32 with C++ front-end PR target/64893 * config/aarch64/aarch64-builtins.c (aarch64_init_simd_builtins): Change the first argument type to size_type_node and add another size_type_node. (aarch64_simd_expand_builtin): Handle the new argument to AARCH64_SIMD_BUILTIN_LANE_CHECK and don't ICE but rather print an out when the first two arguments are not nonzero integer constants. * config/aarch64/arm_neon.h (__AARCH64_LANE_CHECK): Pass the sizeof directly to __builtin_aarch64_im_lane_boundsi. * testsuite/c-c++-common/torture/aarch64-vect-lane-1.c: New testcase. * testsuite/c-c++-common/torture/aarch64-vect-lane-2.c: New testcase. diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c index 87f1ac2..eabf873 100644 --- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -712,7 +712,8 @@ aarch64_init_simd_builtins (void) aarch64_init_simd_builtin_scalar_types (); tree lane_check_fpr = build_function_type_list (void_type_node, - intSI_type_node, + size_type_node, + size_type_node, intSI_type_node, NULL); aarch64_builtin_decls[AARCH64_SIMD_BUILTIN_LANE_CHECK] = @@ -1000,14 +1001,24 @@ rtx aarch64_simd_expand_builtin (int fcode, tree exp, rtx target) { if (fcode == AARCH64_SIMD_BUILTIN_LANE_CHECK) - { - tree nlanes = CALL_EXPR_ARG (exp, 0); - gcc_assert (TREE_CODE (nlanes) == INTEGER_CST); - rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 1)); - if (CONST_INT_P (lane_idx)) - aarch64_simd_lane_bounds (lane_idx, 0, TREE_INT_CST_LOW (nlanes), exp); +{ + rtx totalsize = expand_normal (CALL_EXPR_ARG (exp, 0)); + rtx elementsize = expand_normal (CALL_EXPR_ARG (exp, 1)); + if (CONST_INT_P (totalsize) && CONST_INT_P (elementsize) + && UINTVAL (elementsize) != 0 + && UINTVAL (totalsize) != 0) + { + rtx lane_idx = expand_normal (CALL_EXPR_ARG (exp, 2)); + if (CONST_INT_P (lane_idx)) + aarch64_simd_lane_bounds (lane_idx, 0, + UINTVAL (totalsize) + / UINTVAL (elementsize), + exp); + else + error ("%Klane index must be a constant immediate", exp); + } else - error ("%Klane index must be a constant immediate", exp); + error ("%Ktotal size and element size must be a non-zero constant immediate", exp); /* Don't generate any RTL. */ return const0_rtx; } diff --git a/gcc/config/aarch64/arm_neon.h b/gcc/config/aarch64/arm_neon.h index 2525a27..4c15312 100644 --- a/gcc/config/aarch64/arm_neon.h +++ b/gcc/config/aarch64/arm_neon.h @@ -541,7 +541,7 @@ typedef struct poly16x8x4_t #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) #define __AARCH64_LANE_CHECK(__vec, __idx) \ - __builtin_aarch64_im_lane_boundsi (__AARCH64_NUM_LANES (__vec), __idx) + __builtin_aarch64_im_lane_boundsi (sizeof(__vec), sizeof(__vec[0]), __idx) /* For big-endian, GCC's vector indices are the opposite way around to the architectural lane indices used by Neon intrinsics. */ diff --git a/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c new file mode 100644 index 0000000..1790c34 --- /dev/null +++ b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-1.c @@ -0,0 +1,8 @@ +// { dg-do compile { target "aarch64*-*-*" } } +#include +int +search_line_fast (uint32x2_t t) +{ + return vget_lane_u32 (t, 0); +} + diff --git a/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c new file mode 100644 index 0000000..2950480 --- /dev/null +++ b/gcc/testsuite/c-c++-common/torture/aarch64-vect-lane-2.c @@ -0,0 +1,7 @@ +// { dg-do compile { target "aarch64*-*-*" } } +int +search_line_fast (void) +{ + __builtin_aarch64_im_lane_boundsi (4, 0, 0); /* { dg-error "" } */ +} +