Fix bug in MEM parsing in patches 8a/8b

Testing the patch kit on i686 showed numerous failures of this
assertion in set_mem_attributes_minus_bitpos in emit-rtl.c:

  1821        gcc_assert (!defattrs->offset_known_p);

when expanding "main" in the rtl.exp test files, after parsing
an __RTL-tagged function.

Root cause is various assignments within the RTL parser of the
form:

1222		      MEM_OFFSET (x) = atoi (name.string);

where a MEM_* macro appears on the left-hand side of an assignment.

These macros are defined as a field lookup on the result of a call
to get_mem_attrs, e.g.:

  #define MEM_OFFSET(RTX) (get_mem_attrs (RTX)->offset)

get_mem_attrs can return the struct mem_attrs * of an rtx, but if
it isn't set, it returns:
   mode_mem_attrs[(int) GET_MODE (x)];

which is this field within struct GTY(()) target_rtl:
  /* The default memory attributes for each mode.  */
  struct mem_attrs *x_mode_mem_attrs[(int) MAX_MACHINE_MODE];

These assignments in the parser were erroneously writing to these
default per-mode values, rather than assigning to a unique-per-rtx
instance of struct mem_attrs.

The fix is to call the appropriate set_mem_ functions in the
parser, e.g. set_mem_offset; the patch below is intended as a tweak
to patch 8a of the kit, and would be merged with it before committing.

The patch also adds extra test coverage for MEM parsing.  This extends
the target-independent selftests, and so would go into patch 8b.

Tested for targets x86_64-pc-linux-gnu, i686-pc-linux-gnu,
and aarch64-linux-gnu, and on powerpc-ibm-aix7.1.3.0.

OK as adjustments to patches 8a and 8b?

For patch 8a:
  gcc/ChangeLog:
	* read-rtl-function.c
	(function_reader::handle_any_trailing_information): Replace writes
	through macros MEM_ALIAS_SET, MEM_OFFSET, MEM_SIZE, MEM_ALIGN,
	and MEM_ADDR_SPACE with calls to set_mem_ functions.  Add missing
	call to unread_char when handling "A" for alignment.

For patch 8b:
  gcc/ChangeLog:
	* read-rtl-function.c (selftest::test_loading_mem): New function.
	(selftest::read_rtl_function_c_tests): Call it.
  gcc/testsuite/ChangeLog:
	* selftests/mem.rtl: New file.
---
 gcc/read-rtl-function.c         | 55 ++++++++++++++++++++++++++++++++++++-----
 gcc/testsuite/selftests/mem.rtl |  9 +++++++
 2 files changed, 58 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/selftests/mem.rtl

On 12/08/2016 09:39 PM, David Malcolm wrote:
>
> OK as adjustments to patches 8a and 8b?

Ok. Is it possible to adjust these macros to return something like a 
const reference to ensure people can't make this kind of error?


Bernd

On 12/08/2016 01:39 PM, David Malcolm wrote:
> Testing the patch kit on i686 showed numerous failures of this
> assertion in set_mem_attributes_minus_bitpos in emit-rtl.c:
>
>   1821        gcc_assert (!defattrs->offset_known_p);
>
> when expanding "main" in the rtl.exp test files, after parsing
> an __RTL-tagged function.
>
> Root cause is various assignments within the RTL parser of the
> form:
>
> 1222		      MEM_OFFSET (x) = atoi (name.string);
>
> where a MEM_* macro appears on the left-hand side of an assignment.
>
> These macros are defined as a field lookup on the result of a call
> to get_mem_attrs, e.g.:
>
>   #define MEM_OFFSET(RTX) (get_mem_attrs (RTX)->offset)
>
> get_mem_attrs can return the struct mem_attrs * of an rtx, but if
> it isn't set, it returns:
>    mode_mem_attrs[(int) GET_MODE (x)];
>
> which is this field within struct GTY(()) target_rtl:
>   /* The default memory attributes for each mode.  */
>   struct mem_attrs *x_mode_mem_attrs[(int) MAX_MACHINE_MODE];
>
> These assignments in the parser were erroneously writing to these
> default per-mode values, rather than assigning to a unique-per-rtx
> instance of struct mem_attrs.
>
> The fix is to call the appropriate set_mem_ functions in the
> parser, e.g. set_mem_offset; the patch below is intended as a tweak
> to patch 8a of the kit, and would be merged with it before committing.
>
> The patch also adds extra test coverage for MEM parsing.  This extends
> the target-independent selftests, and so would go into patch 8b.
>
> Tested for targets x86_64-pc-linux-gnu, i686-pc-linux-gnu,
> and aarch64-linux-gnu, and on powerpc-ibm-aix7.1.3.0.
>
> OK as adjustments to patches 8a and 8b?
>
> For patch 8a:
>   gcc/ChangeLog:
> 	* read-rtl-function.c
> 	(function_reader::handle_any_trailing_information): Replace writes
> 	through macros MEM_ALIAS_SET, MEM_OFFSET, MEM_SIZE, MEM_ALIGN,
> 	and MEM_ADDR_SPACE with calls to set_mem_ functions.  Add missing
> 	call to unread_char when handling "A" for alignment.
>
> For patch 8b:
>   gcc/ChangeLog:
> 	* read-rtl-function.c (selftest::test_loading_mem): New function.
> 	(selftest::read_rtl_function_c_tests): Call it.
>   gcc/testsuite/ChangeLog:
> 	* selftests/mem.rtl: New file.
They seem like reasonable adjustments.

I know you posted the cc1 patches to add the RTL front-end.  What's the 
status on this kit?

[ Yes, I keep falling behind... ]

jeff

On Mon, 2016-12-19 at 15:10 -0700, Jeff Law wrote:
> On 12/08/2016 01:39 PM, David Malcolm wrote:
> > Testing the patch kit on i686 showed numerous failures of this
> > assertion in set_mem_attributes_minus_bitpos in emit-rtl.c:
> > 
> >   1821        gcc_assert (!defattrs->offset_known_p);
> > 
> > when expanding "main" in the rtl.exp test files, after parsing
> > an __RTL-tagged function.
> > 
> > Root cause is various assignments within the RTL parser of the
> > form:
> > 
> > 1222		      MEM_OFFSET (x) = atoi (name.string);
> > 
> > where a MEM_* macro appears on the left-hand side of an assignment.
> > 
> > These macros are defined as a field lookup on the result of a call
> > to get_mem_attrs, e.g.:
> > 
> >   #define MEM_OFFSET(RTX) (get_mem_attrs (RTX)->offset)
> > 
> > get_mem_attrs can return the struct mem_attrs * of an rtx, but if
> > it isn't set, it returns:
> >    mode_mem_attrs[(int) GET_MODE (x)];
> > 
> > which is this field within struct GTY(()) target_rtl:
> >   /* The default memory attributes for each mode.  */
> >   struct mem_attrs *x_mode_mem_attrs[(int) MAX_MACHINE_MODE];
> > 
> > These assignments in the parser were erroneously writing to these
> > default per-mode values, rather than assigning to a unique-per-rtx
> > instance of struct mem_attrs.
> > 
> > The fix is to call the appropriate set_mem_ functions in the
> > parser, e.g. set_mem_offset; the patch below is intended as a tweak
> > to patch 8a of the kit, and would be merged with it before
> > committing.
> > 
> > The patch also adds extra test coverage for MEM parsing.  This
> > extends
> > the target-independent selftests, and so would go into patch 8b.
> > 
> > Tested for targets x86_64-pc-linux-gnu, i686-pc-linux-gnu,
> > and aarch64-linux-gnu, and on powerpc-ibm-aix7.1.3.0.
> > 
> > OK as adjustments to patches 8a and 8b?
> > 
> > For patch 8a:
> >   gcc/ChangeLog:
> > 	* read-rtl-function.c
> > 	(function_reader::handle_any_trailing_information): Replace
> > writes
> > 	through macros MEM_ALIAS_SET, MEM_OFFSET, MEM_SIZE, MEM_ALIGN,
> > 	and MEM_ADDR_SPACE with calls to set_mem_ functions.  Add
> > missing
> > 	call to unread_char when handling "A" for alignment.
> > 
> > For patch 8b:
> >   gcc/ChangeLog:
> > 	* read-rtl-function.c (selftest::test_loading_mem): New
> > function.
> > 	(selftest::read_rtl_function_c_tests): Call it.
> >   gcc/testsuite/ChangeLog:
> > 	* selftests/mem.rtl: New file.
> They seem like reasonable adjustments.
> 
> I know you posted the cc1 patches to add the RTL front-end.  What's
> the 
> status on this kit?
> 
> [ Yes, I keep falling behind... ]

Current status of RTL frontend patch kit:

In summary: patch 8d and patch 9 need review.

In detail:

* patches 1-6 of the kit are committed to trunk

* patch 7 (in extremely minimal form) has been approved, merged into
patch 8a.

* patch 8 (adding function_reader class, and selftests to verify it
works) split into four subpatches, not yet in trunk:
  * patches 8a, 8b, and 8c are approved (the reader itself, selftests
for it that don't depend on any target, and selftests that are aarch64
-specific)
  * patches 8d isn't approved yet; I reposted this today as:
    * "[PATCH] Add x86_64-specific selftests for RTL function reader
(v2)"
      https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01616.html
  * I'm successfully run config-list.mk testing across 191 targets to
verify that these selftests don't break the build for anything (there
was some snafu with our dump syntax for pseudos conflicting with hard
reg names on iq2000, but this is resolved now)
  * my plan is to commit 8a-8d as one combined patch once 8d is
approved (assuming it is)

* patch 9 (wiring it up into cc1) needs review:
  * "[PATCH] Add "__RTL" to cc1 (v7)"
    * https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01662.html


Dave