[ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Sergei Organov]

Hello,

All the instrumentation macros look like this when instrumentation is
disabled:

#define CYG_INSTRUMENT_CLOCK(_event_,_arg1_,_arg2_)             \
    CYG_MACRO_START                                             \
    CYG_UNUSED_PARAM(CYG_ADDRWORD, (CYG_ADDRWORD)(_arg1_));     \
    CYG_UNUSED_PARAM(CYG_ADDRWORD, (CYG_ADDRWORD)(_arg2_));     \
    CYG_MACRO_END

The problem is that when either _arg1_ or _arg2_ are volatile, the
CYG_UNUSED_PARAM leaves reference to the argument. I mean gcc still put
instruction(s) that fetch the value of the argument, because volatile
specification prevents it from just ignoring the code. Consider:

int volatile iv;
void foo(void) {
  CYG_UNUSED_PARAM(int, iv);
}

This produces the following code for 'foo':

void foo(void) {
  int tmp1 = iv;
  int tmp2 = tmp1;
  tmp1 = tmp2;
}

and gcc will produce load (or address access) instruction for 'tmp1 = iv'
statement.

Still this is not a very big problem. Just a few unnecessary instructions are
left in the code :-) However, in the case of CYG_INSTRUMENT_CLOCK this brings
much greater problem when compiling for PowerPC with hardware floating
point. The instructions that gcc produces are in fact floating point load
instructions (!), the code is actually in DSR (Cyg_RealTimeClock::dsr), thus
every clock tick ends up in floating point context switch! The excuse for FP
operation is that the reference is to the Cyg_Counter::counter that is 64 bits
(type long long), and it requires only one FP load instruction to access the
whole variable. For reference, the code in the DSR looks like this:

    ...
    Cyg_RealTimeClock *rtc = (Cyg_RealTimeClock *)data;

    CYG_INSTRUMENT_CLOCK( TICK_START,
                          rtc->current_value_lo(),
                          rtc->current_value_hi());
    ...

    CYG_INSTRUMENT_CLOCK( TICK_END,
                          rtc->current_value_lo(),
                          rtc->current_value_hi());

It produces total 4 floating point load instructions for 4 accesses to the
'rtc->counter' :-(

Well, I've tried to remove all the CYG_UNUSED_PARAM from all the macros in the
'instrmnt.h', and got only one compiler warning related to the
CYG_INSTRUMENT_INTR macro. Thus I left only this macro to be like this:

#define CYG_INSTRUMENT_INTR(_event_,_arg1_,_arg2_)              \
    CYG_MACRO_START                                             \
    CYG_UNUSED_PARAM(CYG_ADDRWORD, (CYG_ADDRWORD)(_arg1_));     \
    CYG_MACRO_END

Now there are no compiler warnings and no unnecessary loads in the code.

Do I submit the patch, or is there a better solution for the problem?

BR,
Sergei Organov.

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Nick Garnett]

Sergei Organov <osv@javad.ru> writes:

[CYG_UNUSED_PARAM leaves unneeded code]

> Well, I've tried to remove all the CYG_UNUSED_PARAM from all the macros in the
> 'instrmnt.h', and got only one compiler warning related to the
> CYG_INSTRUMENT_INTR macro. Thus I left only this macro to be like this:
> 
> #define CYG_INSTRUMENT_INTR(_event_,_arg1_,_arg2_)              \
>     CYG_MACRO_START                                             \
>     CYG_UNUSED_PARAM(CYG_ADDRWORD, (CYG_ADDRWORD)(_arg1_));     \
>     CYG_MACRO_END

Which invocation of this macro causes this? The simple solution is to
add a call to CYG_UNUSED_PARAM() just before the instrumentation macro
call and get rid of the call in the macro.

> 
> Now there are no compiler warnings and no unnecessary loads in the code.
> 
> Do I submit the patch, or is there a better solution for the problem?
> 

Having thought about this and discussed this amongst ourselves we
think that removing the CYG_UNUSED_PARAM calls is probably the best
solution.

The CYG_UNUSED_PARAM calls were only added about year ago to fix some
compiler warnings (not by me, so I do not know which platform this
was). So we will just be backing out the change. We may need to add
some call to CYG_UNUSED_PARAM in various places to dispose of the
compiler warnings.

The changes needed here seem to be fairly trivial, so there probably
is no need for you to send us a patch.

-- 
Nick Garnett
Cygnus Solutions, a Red Hat Company
Cambridge, UK

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Sergei Organov]

Nick Garnett <nickg@cygnus.co.uk> writes:

> Sergei Organov <osv@javad.ru> writes:
> 
> [CYG_UNUSED_PARAM leaves unneeded code]
> 
> > Well, I've tried to remove all the CYG_UNUSED_PARAM from all the macros in the
> > 'instrmnt.h', and got only one compiler warning related to the
> > CYG_INSTRUMENT_INTR macro. Thus I left only this macro to be like this:
> > 
> > #define CYG_INSTRUMENT_INTR(_event_,_arg1_,_arg2_)              \
> >     CYG_MACRO_START                                             \
> >     CYG_UNUSED_PARAM(CYG_ADDRWORD, (CYG_ADDRWORD)(_arg1_));     \
> >     CYG_MACRO_END
> 
> Which invocation of this macro causes this? The simple solution is to
> add a call to CYG_UNUSED_PARAM() just before the instrumentation macro
> call and get rid of the call in the macro.

I don't remember, somewhere in the kernel code. I believe you will see 
it after removing CYG_UNUSED_PARAM in the macro.

> 
> > 
> > Now there are no compiler warnings and no unnecessary loads in the code.
> > 
> > Do I submit the patch, or is there a better solution for the problem?
> > 
> 
> Having thought about this and discussed this amongst ourselves we
> think that removing the CYG_UNUSED_PARAM calls is probably the best
> solution.
> 
> The CYG_UNUSED_PARAM calls were only added about year ago to fix some
> compiler warnings (not by me, so I do not know which platform this
> was). So we will just be backing out the change. We may need to add
> some call to CYG_UNUSED_PARAM in various places to dispose of the
> compiler warnings.
>

This seems to be the most reasonable solution for me as well. It's indeed much
easier to fix compiler warnings where they appear (not a lot of places, I
believe) than to find what I've found :-)

> 
> The changes needed here seem to be fairly trivial, so there probably
> is no need for you to send us a patch.

OK, I'll wait until it appears in anon cvs and then revert to the cvs version.

BR,
Sergei.

> -- 
> Nick Garnett
> Cygnus Solutions, a Red Hat Company
> Cambridge, UK

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Jesper Skov]

>The problem is that when either _arg1_ or _arg2_ are volatile, the
>CYG_UNUSED_PARAM leaves reference to the argument. I mean gcc still put
>instruction(s) that fetch the value of the argument, because volatile
>specification prevents it from just ignoring the code. Consider:
>
>int volatile iv;
>void foo(void) {
>  CYG_UNUSED_PARAM(int, iv);
>}
>
>This produces the following code for 'foo':
>
>void foo(void) {
>  int tmp1 = iv;
>  int tmp2 = tmp1;
>  tmp1 = tmp2;
>}
>
>and gcc will produce load (or address access) instruction for 'tmp1 = iv'
>statement.


I just had an idea. Maybe we could change the CYG_UNUSED_PARAM to:

#define CYG_UNUSED_PARAM(_type_, _var_) \
    if (0) { if ((_var_) == (_var_)); }

That would always work without any overhead, wouldn't it?

Jesper

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Sergei Organov]

Jesper Skov <jskov@redhat.com> writes:
[...]
> I just had an idea. Maybe we could change the CYG_UNUSED_PARAM to:
>
> #define CYG_UNUSED_PARAM(_type_, _var_) \
>     if (0) { if ((_var_) == (_var_)); }
>
> That would always work without any overhead, wouldn't it?

Only if optimization is turned on ;-)

This also requires _type_ to have operator '==' (not the case for structs).

The obvious replacement

if (0) { (_var_) = (_var_); }

will sometimes emit floating point instructions on the PPC that in turn may
cause (while probability is very low) saving/restoring of FP registers in the
function prologue/epilogue :-(

I still think that it's better to don't use CYG_UNUSED_PARAM inside macros.

Sergei.

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Jesper Skov]

>>>>> "Sergei" == Sergei Organov <osv@javad.ru> writes:

Sergei> Jesper Skov <jskov@redhat.com> writes: [...]
>> I just had an idea. Maybe we could change the CYG_UNUSED_PARAM to:
>> 
>> #define CYG_UNUSED_PARAM(_type_, _var_) \ if (0) { if ((_var_) ==
>> (_var_)); }
>> 
>> That would always work without any overhead, wouldn't it?

Sergei> Only if optimization is turned on ;-)

That's true. But if compiling without optimization, you probably don't
care about spurious instructions :)

Sergei> This also requires _type_ to have operator '==' (not the case
Sergei> for structs).

Sergei> The obvious replacement

Sergei> if (0) { (_var_) = (_var_); }

What about

#define CYG_UNUSED_PARAM(_type_, _var_) \
  if (0) { if (((CYG_ADDRESS)&(_var_)) != ((CYG_ADDRESS)&(_var_))); }

Is there anything you cannot take the address of?

Jesper

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Sergei Organov]

Jesper Skov <jskov@redhat.com> writes:

[...]
> What about
> 
> #define CYG_UNUSED_PARAM(_type_, _var_) \
>   if (0) { if (((CYG_ADDRESS)&(_var_)) != ((CYG_ADDRESS)&(_var_))); }
> 
> Is there anything you cannot take the address of?

Unfortunately yes. If you declared _var_ to be register:

register int v;

then gcc will print warning if you try to take its address :-(

Sergei.

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Jesper Skov]

>>>>> "Sergei" == Sergei Organov <osv@javad.ru> writes:

Sergei> Jesper Skov <jskov@redhat.com> writes: [...]
>> What about
>> 
>> #define CYG_UNUSED_PARAM(_type_, _var_) \ if (0) { if
>> (((CYG_ADDRESS)&(_var_)) != ((CYG_ADDRESS)&(_var_))); }
>> 
>> Is there anything you cannot take the address of?

Sergei> Unfortunately yes. If you declared _var_ to be register:

Sergei> register int v;

Sergei> then gcc will print warning if you try to take its address :-(

Hmm... Being a hacker sure ain't easy :)

What we really need is some GCC attribute magic one can use to mark an
argument (intentionally) unused. Maybe a project for a rainy Sunday...

Jesper

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Sergei Organov]

Jesper Skov <jskov@redhat.com> writes:
> What we really need is some GCC attribute magic one can use to mark an
> argument (intentionally) unused. Maybe a project for a rainy Sunday...

The problem is that when used in macros, the argument isn't always an argument
:-) When it's an argument, the usual practice with gcc is just

void foo(int arg) {
  (void)arg;
}

but it still produces memory access in the case (that we can get when we use
it in macros):

extern int volatile arg;
void foo(void) {
  (void)arg;
}

Usually you mark external variables as unused in the function by don't using
them, so it seems too much to ask gcc to have some attribute magic for it :-)

Here is my bet:

#define CYG_UNUSED_PARAM(_type_, _var_) {if(0)(void)(var);}

but I'm afraid somebody else will find a case where this doesn't work anyway
:-( Note that in this definition 'type' is also unused, and thus must be
declared as such inside the macro. This is another interesting project for
a rainy Sunday, I believe :-)

Sergei.

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Bart Veer]

    >>> What about
    >>> 
    >>> #define CYG_UNUSED_PARAM(_type_, _var_) \ if (0) { if
    >>> (((CYG_ADDRESS)&(_var_)) != ((CYG_ADDRESS)&(_var_))); }
    >>> 
    >>> Is there anything you cannot take the address of?

    Sergei> Unfortunately yes. If you declared _var_ to be register:

    Sergei> register int v;

    Sergei> then gcc will print warning if you try to take its address :-(

    Jesper> Hmm... Being a hacker sure ain't easy :)

    Jesper> What we really need is some GCC attribute magic one can
    Jesper> use to mark an argument (intentionally) unused. Maybe a
    Jesper> project for a rainy Sunday...

From the gcc info page:

`unused'
     This attribute, attached to a variable, means that the variable is
     meant to be possibly unused.  GNU CC will not produce a warning
     for this variable.

However I have never tried using it so I do not know how well it would
serve our purposes.

Bart

Re: [ECOS] CYG_INSTRUMENT_CLOCK+PowerPC+FPU problem.

[Nick Garnett]

Sergei Organov <osv@javad.ru> writes:

[CYG_UNUSED_PARAM leaves unneeded code]

> Well, I've tried to remove all the CYG_UNUSED_PARAM from all the macros in the
> 'instrmnt.h', and got only one compiler warning related to the
> CYG_INSTRUMENT_INTR macro. Thus I left only this macro to be like this:
> 
> #define CYG_INSTRUMENT_INTR(_event_,_arg1_,_arg2_)              \
>     CYG_MACRO_START                                             \
>     CYG_UNUSED_PARAM(CYG_ADDRWORD, (CYG_ADDRWORD)(_arg1_));     \
>     CYG_MACRO_END

Which invocation of this macro causes this? The simple solution is to
add a call to CYG_UNUSED_PARAM() just before the instrumentation macro
call and get rid of the call in the macro.

> 
> Now there are no compiler warnings and no unnecessary loads in the code.
> 
> Do I submit the patch, or is there a better solution for the problem?
> 

Having thought about this and discussed this amongst ourselves we
think that removing the CYG_UNUSED_PARAM calls is probably the best
solution.

The CYG_UNUSED_PARAM calls were only added about year ago to fix some
compiler warnings (not by me, so I do not know which platform this
was). So we will just be backing out the change. We may need to add
some call to CYG_UNUSED_PARAM in various places to dispose of the
compiler warnings.

The changes needed here seem to be fairly trivial, so there probably
is no need for you to send us a patch.

-- 
Nick Garnett
Cygnus Solutions, a Red Hat Company
Cambridge, UK