On Tue, May 8, 2012 at 5:39 AM, Hans-Peter Nilsson
<hans-peter.nils...@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
