On Fri, May 29, 2015 at 11:57:00AM +0100, Dominik Vogt wrote:
> 2015-05-29 Dominik Vogt <v...@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
