From: Richard Biener <richard.guenther@gmail.com>
To: gcc-patches@gcc.gnu.org,James Greenhalgh
<James.Greenhalgh@arm.com>,Martin Sebor <msebor@gmail.com>
Cc: Jason Merrill <jason@redhat.com>,Gcc Patch List
<gcc-patches@gcc.gnu.org>,Joseph Myers
<joseph@codesourcery.com>,nd <nd@arm.com>
Subject: Re: [PATCH] convert braced initializers to strings (PR 71625)
Date: Wed, 15 Aug 2018 15:04:00 -0000 [thread overview]
Message-ID: <492BAF23-60C7-4245-A7AB-B91E6AD2C25A@gmail.com> (raw)
In-Reply-To: <AM4PR0802MB2370864417A0C355BAA5109A843F0@AM4PR0802MB2370.eurprd08.prod.outlook.com>
On August 15, 2018 12:28:55 PM GMT+02:00, James Greenhalgh <James.Greenhalgh@arm.com> wrote:
>On Tue, Aug 14, 2018 at 09:34:08PM -0500, Martin Sebor wrote:
>> On 08/14/2018 09:24 AM, Martin Sebor wrote:
>> > On 08/14/2018 09:08 AM, Martin Sebor wrote:
>> >> On 08/14/2018 07:27 AM, James Greenhalgh wrote:
>> >>> On Wed, Aug 08, 2018 at 07:17:07PM -0500, Martin Sebor wrote:
>> >>>> On 08/08/2018 05:08 AM, Jason Merrill wrote:
>> >>>>> On Wed, Aug 8, 2018 at 9:04 AM, Martin Sebor <msebor@gmail.com>
>wrote:
>> >>>>>> On 08/07/2018 02:57 AM, Jason Merrill wrote:
>> >>>>>>>
>> >>>>>>> On Wed, Aug 1, 2018 at 12:49 AM, Martin Sebor
><msebor@gmail.com>
>> >>>>>>> wrote:
>> >>>>>>>>
>> >>>>>>>> On 07/31/2018 07:38 AM, Jason Merrill wrote:
>> >>>
>> >>> <snip>
>> >>>
>> >>>> Done in the attached patch. I've also avoided dealing with
>> >>>> zero-length arrays and added tests to make sure their size
>> >>>> stays is regardless of the form of their initializer and
>> >>>> the appropriate warnings are issued.
>> >>>>
>> >>>> Using build_string() rather than build_string_literal() needed
>> >>>> a tweak in digest_init_r(). It didn't break anything but since
>> >>>> the array type may not have a domain yet, neither will the
>> >>>> string. It looks like that may get adjusted later on but I've
>> >>>> temporarily guarded the code with #if 1. If the change is
>> >>>> fine I'll remove the #if before committing.
>> >>>>
>> >>>> This initial patch only handles narrow character initializers
>> >>>> (i.e., those with TYPE_STRING_FLAG set). Once this gets some
>> >>>> exposure I'd like to extend it to other character types,
>> >>>> including wchar_t.
>> >>>
>> >>> Hi Martin,
>> >>>
>> >>> This causes issues for the AArch64 tests (full list below).
>> >>>
>> >>> I see an error message on the following construct:
>> >>>
>> >>> void foo (void)
>> >>> {
>> >>> __Poly8_t x[4] = { 1, 2, 3, 4 };
>> >>> }
>> >>>
>> >>> init.c:3:20: error: array of inappropriate type initialized
>from
>> >>> string constant
>> >>> 3 | __Poly8_t x[4] = { 1, 2, 3, 4 };
>> >>> |
>> >>>
>> >>> __Poly8_t is a type we define in our backend, through a
>convoluted
>> >>> set of
>> >>> functions, which operates a lot like an unsigned, QI mode type.
>> >>
>> >> I see the error with my aarch64 cross-compiler . The new code
>> >> that does the conversion of array initializers to STRING_CSTs
>> >> looks for the TYPE_STRING_FLAG() to be set on the type of
>> >> the array elements. Perhaps __Poly8_t should not have the flag
>> >> set? (If it needs it then I think we'd have to only consider
>> >> named character types.)
>> >
>> > The change below gets rid of the compilation error. I don't
>> > know if it's appropriate for the aarch64 back end:
>> >
>> > Index: gcc/config/aarch64/aarch64-builtins.c
>> > ===================================================================
>> > --- gcc/config/aarch64/aarch64-builtins.c (revision 263537)
>> > +++ gcc/config/aarch64/aarch64-builtins.c (working copy)
>> > @@ -643,6 +643,7 @@ aarch64_init_simd_builtin_types (void)
>> > /* Poly types are a world of their own. */
>> > aarch64_simd_types[Poly8_t].eltype =
>aarch64_simd_types[Poly8_t].itype =
>> > build_distinct_type_copy (unsigned_intQI_type_node);
>> > + TYPE_STRING_FLAG (aarch64_simd_types[Poly8_t].eltype) = false;
>> > aarch64_simd_types[Poly16_t].eltype =
>> > aarch64_simd_types[Poly16_t].itype =
>> > build_distinct_type_copy (unsigned_intHI_type_node);
>> > aarch64_simd_types[Poly64_t].eltype =
>> > aarch64_simd_types[Poly64_t].itype =
>
>This fix seems correct to me, the poly types are not strings. Looking
>at
>other uses of TYPE_STRING_FLAG this change doesn't seem like it would
>have
>impact on parsing or code generation.
>
>OK for trunk.
>
>> >>> A second set of tests fail due to changed inlining behaviour for
>> >>> functions
>> >>> with char array initialization:
>> >>>
>> >>> gcc.target/aarch64/vset_lane_1.c
>> >>> gcc.target/aarch64/vneg_s.c
>> >>> gcc.target/aarch64/vclz.c
>> >>
>> >> I'm not sure what's going on here. The tests are very big and
>> >> take forever to compile with an aarch64 cross-compiler, and I'm
>> >> not sure what to look for. Can you provide a smaller test case
>> >> that shows the issue?
>>
>> I wonder if these changes might be due to the same problem:
>> the tests define and initialize arrays of the Int8x16_t type
>> which is initialized to intQI_type_node, i.e., the signed
>> form of Poly8_t. Does the conversion to STRING_CST cause
>> a performance degradation or is it just that the tests end
>> up with equivalent but slightly different assembly?
>
>These tests aren't looking at performance, just expecting to see
>certain
>instructions emitted. The only change is that now the int8x16_t forms
>are
>inlined (so the scan-assembler-times fails with two matches, one
>expected,
>one in the inlined function body copy).
>
>The difference seems to be in the initialization cost of the input data
>set.
>
>Before your patch:
>
> int8_tD.3359 test_set0D.21541[8];
> int8_tD.3359 answ_set0D.21542[8];
>
> test_set0D.21541[0] = 0;
> test_set0D.21541[1] = 1;
> test_set0D.21541[2] = -1;
> test_set0D.21541[3] = 10;
> test_set0D.21541[4] = -10;
> test_set0D.21541[5] = 0;
> test_set0D.21541[6] = 127;
> test_set0D.21541[7] = -128;
> answ_set0D.21542[0] = 0;
> answ_set0D.21542[1] = -1;
> answ_set0D.21542[2] = 1;
> answ_set0D.21542[3] = -10;
> answ_set0D.21542[4] = 10;
> answ_set0D.21542[5] = 0;
> answ_set0D.21542[6] = -127;
> answ_set0D.21542[7] = -128;
>
>After your patch:
>
> int8_tD.3357 test_set0D.21539[8];
> int8_tD.3357 answ_set0D.21540[8];
>
> test_set0D.21539 = "";
> answ_set0D.21540 = "";
Can we fix printing of string literals to include non-printable chars please? The above looks like wrong-code though it probably isn't. When Martin L. proposed a similar patch I was suggesting to restrict conversion to initializes with only printable characters.
>I think that is probably what you expected to happen; but the impact on
>inlining might not have been.
It's more the effect of changed gimplification that shows, the inlining is a 2nd order effect.
Richard.
Probably, we want to just change these
>tests
>to explicitly disable inlining. The tests appear to execute correctly.
>
>The print in the dump file is a bit unusual; presumably the impact of
>having
>non-printing characters in my initializer list - but less helpful
>output for
>it.
>
>Off topic; these tests are quick to copmpile on my cross and native
>compilers. Do you have additional checking enabled?
>
>Thanks,
>James
>
>>
>> The tests also use int8_t and uint8_t for the expected results.
>> Those are typedefs for signed and unsigned char, respectively.
>> Is the conversion to strings for those fine?
>>
>> Martin
next prev parent reply other threads:[~2018-08-15 15:04 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-30 23:51 Martin Sebor
2018-07-31 13:39 ` Jason Merrill
2018-07-31 14:49 ` Martin Sebor
2018-08-07 8:58 ` Jason Merrill
2018-08-07 23:04 ` Martin Sebor
2018-08-08 11:09 ` Jason Merrill
2018-08-09 0:17 ` Martin Sebor
2018-08-09 22:07 ` Joseph Myers
2018-08-13 10:36 ` Jason Merrill
2018-08-14 13:27 ` James Greenhalgh
2018-08-14 15:08 ` Martin Sebor
2018-08-14 15:24 ` Martin Sebor
2018-08-15 2:34 ` Martin Sebor
2018-08-15 10:29 ` James Greenhalgh
2018-08-15 15:04 ` Richard Biener [this message]
2018-08-15 15:51 ` Martin Sebor
2018-08-17 17:19 ` [PATCH] Fix poly types after PR tree-optimization/71625 strlen optimization Szabolcs Nagy
2018-08-17 17:22 ` Kyrill Tkachov
2018-08-14 21:14 ` [PATCH] convert braced initializers to strings (PR 71625) Joseph Myers
2018-08-14 22:18 ` Martin Sebor
2018-08-15 12:07 ` Joseph Myers
2018-08-15 21:02 ` Martin Sebor
2018-08-15 21:14 ` Jeff Law
2018-08-15 21:34 ` Jeff Law
2018-08-16 15:23 ` Martin Sebor
2018-08-16 15:32 ` Jeff Law
2018-08-06 16:41 ` Martin Sebor
2018-08-06 17:04 ` Joseph Myers
2018-08-07 2:02 ` Martin Sebor
2018-08-07 11:31 ` Joseph Myers
2018-08-07 23:05 ` Martin Sebor
2018-08-08 17:33 Bernd Edlinger
2018-08-08 18:34 ` Bernd Edlinger
2018-08-08 19:50 ` Martin Sebor
2018-08-08 20:35 ` Bernd Edlinger
2018-08-08 20:48 ` Joseph Myers
2018-08-09 0:23 ` Martin Sebor
2018-08-09 10:47 ` Joseph Myers
2018-08-16 6:37 Bernd Edlinger
2018-08-17 22:26 ` Bernd Edlinger
2018-08-18 4:13 ` Jeff Law
2018-08-18 10:40 ` Richard Sandiford
2018-08-18 14:25 ` Bernd Edlinger
2018-08-18 16:46 ` Richard Sandiford
2018-08-18 17:09 ` Bernd Edlinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=492BAF23-60C7-4245-A7AB-B91E6AD2C25A@gmail.com \
--to=richard.guenther@gmail.com \
--cc=James.Greenhalgh@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=joseph@codesourcery.com \
--cc=msebor@gmail.com \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).