This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] convert braced initializers to strings (PR 71625)


On 08/15/2018 04:28 AM, James Greenhalgh 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.

I committed this in r263561.

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 = "";

I think that is probably what you expected to happen; but the impact on
inlining might not have been. 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.

I agree it's less than helpful.  It's not the result of this
change.  The output is the same for string literals.  E.g., for
this:

  void f (void*, ...);

  void g (void)
  {
    char a[] = { 0, 1, 2, 3 };
    char b[] = "\000\001\002\003";
    f (a, b);
  }

we get the following on trunk:

  <bb 2> [local count: 1073741825]:
  a = "";
  b = "";
  f (&a, &b);

and the following with GCC 8:

  MEM[(char[4] *)&a] = 50462976;
  b = "";
  f (&a, &b);

I'll see if I can quickly tweak things to include all characters
in the output.

Off topic; these tests are quick to copmpile on my cross and native
compilers. Do you have additional checking enabled?

Yes, I did have --enable-checking=all set that I forgot about.
With it, gcc.target/aarch64/advsimd-intrinsics/vmax.c takes
2m 21sec to compile without optimization.  The
gcc.target/aarch64/vclz.c test takes 1m 45sec at -O3.

Removing --enable-checking=all reduced the compile times for
the two tests to just over a second.

Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]