From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45356 invoked by alias); 17 Jul 2015 08:32:56 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 45345 invoked by uid 89); 17 Jul 2015 08:32:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 17 Jul 2015 08:32:54 +0000 Received: from arm.com (e106375-lin.cambridge.arm.com [10.2.207.23]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id t6H8WoiM012968; Fri, 17 Jul 2015 09:32:50 +0100 Date: Fri, 17 Jul 2015 08:38:00 -0000 From: James Greenhalgh To: Charles Baylis Cc: Alan Lawrence , GCC Patches , Tejas Belagod , Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH v3] [AArch64] PR63870 Improve error messages for NEON single lane memory access intrinsics Message-ID: <20150717083250.GA6089@arm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg01494.txt.bz2 On Fri, Jun 26, 2015 at 08:14:55PM +0100, Charles Baylis wrote: > Since the last ping, I've tweaked the test cases a bit... > > Since I've been working on doing the same changes for the ARM backend, > I've moved the tests into the advsimd-intrinsics directory, marked as > XFAIL for ARM targets for now. The gcc/ part of the patch is > unchanged. Hi Charles, This patch looks OK to me, though please fix the whitespace nits called out below: > --- a/gcc/config/aarch64/aarch64-simd.md > +++ b/gcc/config/aarch64/aarch64-simd.md > @@ -3959,7 +3962,7 @@ > (define_insn "vec_store_lanesoi_lane" > [(set (match_operand: 0 "aarch64_simd_struct_operand" "=Utv") > (unspec: [(match_operand:OI 1 "register_operand" "w") > - (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY) > + (unspec:VALLDIF [(const_int 0)] UNSPEC_VSTRUCTDUMMY) > (match_operand:SI 2 "immediate_operand" "i")] > UNSPEC_ST2_LANE))] 8 Spaces to tab. > "TARGET_SIMD" > @@ -3967,7 +3970,7 @@ > operands[2] = GEN_INT (ENDIAN_LANE_N (mode, INTVAL (operands[2]))); > return "st2\\t{%S1. - %T1.}[%2], %0"; > } > - [(set_attr "type" "neon_store3_one_lane")] > + [(set_attr "type" "neon_store2_one_lane")] I would prefer this in a separate patch as it is a separate logical change. Consider it pre-approved (and obvious) to commit as a one-line fix on its own. > @@ -4054,7 +4060,7 @@ > (define_insn "vec_store_lanesci_lane" > [(set (match_operand: 0 "aarch64_simd_struct_operand" "=Utv") > (unspec: [(match_operand:CI 1 "register_operand" "w") > - (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY) > + (unspec:VALLDIF [(const_int 0)] UNSPEC_VSTRUCTDUMMY) > (match_operand:SI 2 "immediate_operand" "i")] > UNSPEC_ST3_LANE))] 8 Spaces to tab. > @@ -4149,7 +4158,7 @@ > (define_insn "vec_store_lanesxi_lane" > [(set (match_operand: 0 "aarch64_simd_struct_operand" "=Utv") > (unspec: [(match_operand:XI 1 "register_operand" "w") > - (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY) > + (unspec:VALLDIF [(const_int 0)] UNSPEC_VSTRUCTDUMMY) > (match_operand:SI 2 "immediate_operand" "i")] 8 Spaces to tab. > diff --git a/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vld2_lane_f32_indices_1.c b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vld2_lane_f32_indices_1.c > new file mode 100644 > index 0000000..04be713 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vld2_lane_f32_indices_1.c > @@ -0,0 +1,16 @@ > +#include > + > +/* { dg-do compile } */ > +/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } } */ This seems an odd limitation, presumably this is a side effect of waiting until expand time to throw an error... It does suggest that we're tackling the problem in the wrong way by pushing this to so late in the compilation pipeline. The property here is on a type itself, which must take a constant value within a given range. That feels much more like the sort of thing we should be detecting and bailing out on closer to the front-end - perhaps with a more generic extension allowing you to annotate any type with an expected/required range (both as a helping hand for VRP and as a way to express programmer defined preconditions). But, given that adding such an extension is likely more effort than needed I think this is OK for now! Cheers, James