Heads-up, PR53273: testsuite separation and dilution problem. Fix for PR53272

The problem was spotted while fixing PR53272, a target bug with
crisv32-* involving the error-prone notice_update_cc function.

When wrapping up the test-case to use as a run-test, adding main
and auxiliary functions to the reduced test-case unexpectedly
made the bug go away.  This despite all functions (except main)
being decorated with noinline, noclone and the special marker
asm ("") ad finitum.  See below: putting the two-file test-case
in a single file causes different code for the
rtc_update_irq_enable function in the .expand stage already.
That REALLY shouldn't happen.  I hope this is just a bug and not
as it's supposed to be, but this is not the first time I notice
this general problem, hence this rant and PR53273:

It is, and you don't understand how this can be a problem, or
think I should just add the brand new function attribute
nofrobnicate?  Well, having to add two files for each test-case
is enough of a problem on its own.  The bigger problems is the
integrity of test-cases: there's no way to know that a future
optimization doesn't see through those separated files (like, an
LTO that is enabled always).

There *must* be a future-proof way to write test-cases marking
where cross-function optimizations should not happen.  If it's
implemented as function-attribute-nofrobnicate so be it, but it
must not be limited to only the optimizations in place today.
Otherwise, some new middle-end generic optimization will
optimize away the test-case (and always return success), most
likely eliminating the point of the test.  When the point of the
test-case is to cover code in a port or lower levels, optimizing
away the test-cases opens up for bugs to silently creep in; the
original bug or bugs in the functionality being covered.  This
optimization-limiting mechanism really should, almost-must, work
within a single file.  In a (semi-)perfect world, someone would
interate over the testsuite, adding such attributes to the
existing tests; I fear a lot of those that don't use "noclone"
are silently already just eliminated to "exit (0)".  We're on a
slippery slope here: it started with having to add "noinline"
attributes, then "noclone" attributes, then the asm("") marker.
Now that doesn't work anymore either.  Can we just have a way to
limit those pesky cross-function optimizations and all their kin
once and for all?

Ok, enough ranting.  If anyone knows off-hand why the code would
differ, feel free to add to PR53273, else I'll eventually
analyze it.  It might just be a minor bug after all; like some
static branch prediction not being cleared when seeing a
noinline-marked function.

The test-case below and patch will be committed to trunk and the
4.7 branch as soon as testing for crisv32-elf finishes.

gcc/testsuite:
	PR target/53272
	* gcc.dg/torture/pr53272-1.c, gcc.dg/torture/pr53272-2.c: New test.

gcc:
	PR target/53272
	* config/cris/cris.c (cris_normal_notice_update_cc): For TARGET_V32,
	when a constant source operand matches an "I" constraint, the "no
	CC0 change" applies to a register-destination only, not a
	strict_low_part-destination.



brgds, H-P

On Tue, May 8, 2012 at 5:39 AM, Hans-Peter Nilsson
<hans-peter.nilsson@axis.com> wrote:
> The problem was spotted while fixing PR53272, a target bug with
> crisv32-* involving the error-prone notice_update_cc function.
>
> When wrapping up the test-case to use as a run-test, adding main
> and auxiliary functions to the reduced test-case unexpectedly
> made the bug go away.  This despite all functions (except main)
> being decorated with noinline, noclone and the special marker
> asm ("") ad finitum.  See below: putting the two-file test-case
> in a single file causes different code for the
> rtc_update_irq_enable function in the .expand stage already.
> That REALLY shouldn't happen.  I hope this is just a bug and not
> as it's supposed to be, but this is not the first time I notice
> this general problem, hence this rant and PR53273:
>
> It is, and you don't understand how this can be a problem, or
> think I should just add the brand new function attribute
> nofrobnicate?  Well, having to add two files for each test-case
> is enough of a problem on its own.  The bigger problems is the
> integrity of test-cases: there's no way to know that a future
> optimization doesn't see through those separated files (like, an
> LTO that is enabled always).
>
> There *must* be a future-proof way to write test-cases marking
> where cross-function optimizations should not happen.  If it's
> implemented as function-attribute-nofrobnicate so be it, but it
> must not be limited to only the optimizations in place today.
> Otherwise, some new middle-end generic optimization will
> optimize away the test-case (and always return success), most
> likely eliminating the point of the test.  When the point of the
> test-case is to cover code in a port or lower levels, optimizing
> away the test-cases opens up for bugs to silently creep in; the
> original bug or bugs in the functionality being covered.  This
> optimization-limiting mechanism really should, almost-must, work
> within a single file.  In a (semi-)perfect world, someone would
> interate over the testsuite, adding such attributes to the
> existing tests; I fear a lot of those that don't use "noclone"
> are silently already just eliminated to "exit (0)".  We're on a
> slippery slope here: it started with having to add "noinline"
> attributes, then "noclone" attributes, then the asm("") marker.
> Now that doesn't work anymore either.  Can we just have a way to
> limit those pesky cross-function optimizations and all their kin
> once and for all?

You don't say what actually is different when you add these functions.
There should be no IPA optimizations possible unless you tell GCC
that it sees the whole program (which means using -flto with the
linker plugin).  That is, marking functions noclone and noinline and
avoiding declaring them static should be enough.

Still some pieces of GCC may expose different code generation due to
DECL uid differences - which, as DECL uids are global, makes extra
functions possibly result in different code for unchanged functions.  That's
generally not wanted but it can happen (similar for other such kinds of
numbers).

Richard.

> Ok, enough ranting.  If anyone knows off-hand why the code would
> differ, feel free to add to PR53273, else I'll eventually
> analyze it.  It might just be a minor bug after all; like some
> static branch prediction not being cleared when seeing a
> noinline-marked function.
>
> The test-case below and patch will be committed to trunk and the
> 4.7 branch as soon as testing for crisv32-elf finishes.
>
> gcc/testsuite:
>        PR target/53272
>        * gcc.dg/torture/pr53272-1.c, gcc.dg/torture/pr53272-2.c: New test.
>
> gcc:
>        PR target/53272
>        * config/cris/cris.c (cris_normal_notice_update_cc): For TARGET_V32,
>        when a constant source operand matches an "I" constraint, the "no
>        CC0 change" applies to a register-destination only, not a
>        strict_low_part-destination.
>
>
> --- /dev/null   Tue Oct 29 15:57:07 2002
> +++ gcc/testsuite/gcc.dg/torture/pr53272-1.c    Tue May  8 03:07:52 2012
> @@ -0,0 +1,39 @@
> +/* { dg-do run } */
> +/* { dg-additional-sources "pr53272-2.c" } */
> +struct rtc_class_ops {
> + int (*f)(void *, unsigned int enabled);
> +};
> +
> +struct rtc_device
> +{
> + void *owner;
> + const struct rtc_class_ops *ops;
> + int ops_lock;
> +};
> +
> +__attribute__ ((__noinline__, __noclone__))
> +extern int foo(void *);
> +__attribute__ ((__noinline__, __noclone__))
> +extern void foobar(void *);
> +
> +__attribute__ ((__noinline__, __noclone__))
> +int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled)
> +{
> + int err;
> + asm volatile ("");
> +
> + err = foo(&rtc->ops_lock);
> +
> + if (err)
> +  return err;
> +
> + if (!rtc->ops)
> +  err = -19;
> + else if (!rtc->ops->f)
> +  err = -22;
> + else
> +  err = rtc->ops->f(rtc->owner, enabled);
> +
> + foobar(&rtc->ops_lock);
> + return err;
> +}
> --- /dev/null   Tue Oct 29 15:57:07 2002
> +++ gcc/testsuite/gcc.dg/torture/pr53272-2.c    Tue May  8 02:23:21 2012
> @@ -0,0 +1,39 @@
> +__attribute__ ((__noinline__, __noclone__))
> +int foo(void *x)
> +{
> +  asm ("");
> +  return *(int *) x != 42;
> +}
> +
> +__attribute__ ((__noinline__, __noclone__))
> +void foobar(void *x)
> +{
> +  asm ("");
> +  if (foo(x))
> +    __builtin_abort();
> +}
> +
> +struct rtc_class_ops {
> + int (*f)(void *, unsigned int enabled);
> +};
> +
> +struct rtc_device
> +{
> + void *owner;
> + struct rtc_class_ops *ops;
> + int ops_lock;
> +};
> +
> +extern __attribute__ ((__noinline__, __noclone__))
> +int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int);
> +
> +int main(void)
> +{
> + struct rtc_class_ops ops = {(void *) 0};
> +  struct rtc_device dev1 = {0, &ops, 42};
> +
> +  if (rtc_update_irq_enable (&dev1, 1) != -22)
> +    __builtin_abort ();
> +
> +  __builtin_exit (0);
> +}
>
> Index: gcc/config/cris/cris.c
> ===================================================================
> --- gcc/config/cris/cris.c      (revision 187269)
> +++ gcc/config/cris/cris.c      (working copy)
> @@ -1695,6 +1695,7 @@ cris_normal_notice_update_cc (rtx exp, r
>                       && (REGNO (SET_SRC (exp))
>                           > CRIS_LAST_GENERAL_REGISTER))
>                   || (TARGET_V32
> +                      && REG_P (SET_DEST (exp))
>                       && satisfies_constraint_I (SET_SRC (exp))))
>            {
>              /* There's no CC0 change for this case.  Just check
>
> brgds, H-P