Re: [PATCH] PR 66215: S390: Fix placement of post-label NOPs with -mhotpatch

[Jakub Jelinek <jakub@redhat.com>]

On Fri, May 29, 2015 at 11:57:00AM +0100, Dominik Vogt wrote:
> 2015-05-29  Dominik Vogt  <vogt@linux.vnet.ibm.com>
> 
> 	PR target0/66215

Please remove the 0 above.

> 	* gcc.target/s390/hotpatch-1.c: Improve to detect multi-line patterns.
> 	* gcc.target/s390/hotpatch-2.c: Likewise.
> 	* gcc.target/s390/hotpatch-3.c: Likewise.
> 	* gcc.target/s390/hotpatch-4.c: Likewise.
> 	* gcc.target/s390/hotpatch-5.c: Likewise.
> 	* gcc.target/s390/hotpatch-6.c: Likewise.
> 	* gcc.target/s390/hotpatch-7.c: Likewise.
> 	* gcc.target/s390/hotpatch-8.c: Likewise.
> 	* gcc.target/s390/hotpatch-9.c: Likewise.
> 	* gcc.target/s390/hotpatch-14.c: Likewise.
> 	* gcc.target/s390/hotpatch-15.c: Likewise.
> 	* gcc.target/s390/hotpatch-16.c: Likewise.
> 	* gcc.target/s390/hotpatch-19.c:Likewise.
> 	* gcc.target/s390/hotpatch-25.c:Likewise.

Missing spaces after : .  Furthermore, it is very unusual
to list the same file more than once in the ChangeLog of the
same patch.  And the descriptions really don't match the changes
in the tests (e.g. hotpatch-1.c only has the -O* removal,
and -13 has also a dg-final line removal, ditto -25).  From what
I can see, some tests are new, those are ok in the ChangeLog as
is (except that hotpatch-28.c is missing),
in other tests you've removed optimization options from dg-options,
and in some tests tweaked scan-assembler regexps, and in others
removed some dg-final directives.
So IMHO you want:

	* gcc.target/s390/hotpatch-1.c: Remove optimization options from
	dg-options.
	* gcc.target/s390/hotpatch-10.c: Likewise.
	* gcc.target/s390/hotpatch-11.c: Likewise.
	* gcc.target/s390/hotpatch-12.c: Likewise.
	* gcc.target/s390/hotpatch-17.c: Likewise.
	* gcc.target/s390/hotpatch-18.c: Likewise.
	* gcc.target/s390/hotpatch-20.c: Likewise.
	* gcc.target/s390/hotpatch-21.c: Likewise.
	* gcc.target/s390/hotpatch-22.c: Likewise.
	* gcc.target/s390/hotpatch-23.c: Likewise.
	* gcc.target/s390/hotpatch-24.c: Likewise.
	* gcc.target/s390/hotpatch-2.c: Likewise.  Adjust scan-assembler
	to check for the exact nops too.
	* gcc.target/s390/hotpatch-3.c: Likewise.
	* gcc.target/s390/hotpatch-4.c: Likewise.
	* gcc.target/s390/hotpatch-5.c: Likewise.
	* gcc.target/s390/hotpatch-6.c: Likewise.
	* gcc.target/s390/hotpatch-7.c: Likewise.
	* gcc.target/s390/hotpatch-8.c: Likewise.
	* gcc.target/s390/hotpatch-9.c: Likewise.
	* gcc.target/s390/hotpatch-14.c: Likewise.
	* gcc.target/s390/hotpatch-15.c: Likewise.
	* gcc.target/s390/hotpatch-16.c: Likewise.
	* gcc.target/s390/hotpatch-19.c: Likewise.
	* gcc.target/s390/hotpatch-25.c: Likewise.  Remove
	scan-assembler-times counting number of .align directives.
	* gcc.target/s390/hotpatch-13.c: Remove optimization options from
	dg-options.  Remove scan-assembler-times counting number of .align
	directives.
	* gcc.target/s390/hotpatch-26.c: New file.
	* gcc.target/s390/hotpatch-27.c: New file.
	* gcc.target/s390/hotpatch-28.c: New file.
	* gcc.target/s390/s390.exp: Run hotpatch-*.c tests as torture tests
	using -Os -O0 -O1 -O2 -O3 options.

> +	/* Emit NOPs
> +	 *  1. inside the area covered by debug information to allow setting
> +	 *     breakpoints at the NOPs,
> +	 *  2. before any insn which results in an asm instruction,
> +	 *  3. before in-function labels to avoid jumping to the NOPs, for
> +	 *     example as part of a loop,
> +	 *  4. before any barrier in case the function is completely empty
> +	 *     (gcc_unreachable ()) and has neither internal labels nor active
> +	 *     insns.  */

I believe the the above comment isn't formatted like comments in GCC usually
are, can you please replace the *s at the beginning of lines (after
whitespace) with spaces?  Also, please use __builtin_unreachable ()
instead of gcc_unreachable ().  No need to retest the patch for ChangeLog
and comment only changes.

And I'll defer the final ack to s390 maintainers.

	Jakub