I think there is something wrong with the definition of P1IN.
Looks like "#define P1IN 0"
Maybe somewhere in your header files is a mistake that overwrites P1IN.
M.
"ratishpunnoose" :
>
>> I don't think we have missed anything - I have tested your code with
>> several gcc compilers (new and old msp430 compilers, several AVR
>> compilers, and gcc for the ColdFire - I have a batch file setup that I
>> use for this sort of testing). They all agree with our interpretation
>> of the code.
>>
>> However, I suggest you check it with IAR. Either it /is/ a bug, or
>> they know more about the interpretation of horrible C code than we do
>> :-)
>> Thanks very much David, I'll do that. I refactored the code into the
> following easy to understand snippet and it exhibits the same problem.
> -------
> static int log_calls(int num_iters)
> {
> puts("\nlogging");
> return (num_iters-1);
> }
>
> void myfunc(void)
> {
> signed int remaining_calls = 1;
> while(P1IN==0)
> {
> remaining_calls = log_calls(remaining_calls);
> if (remaining_calls > 0 )
> {
> continue;
> } else {
> break;
> }
>
> }
> }
> -------
>
>
A compiler optimization puzzle
Started by ●March 1, 2011
Reply by ●March 3, 20112011-03-03
Reply by ●March 3, 20112011-03-03
Reply by ●March 3, 20112011-03-03
On 03/03/2011 16:21, Matthias Weingart wrote:
> I think there is something wrong with the definition of P1IN.
> Looks like "#define P1IN 0"
>
P1IN is a register - it should be defined in a chip-specific header as a
"volatile uint8_t", or equivalent.
mvh.,
David
> Maybe somewhere in your header files is a mistake that overwrites P1IN.
>
> M.
>
> "ratishpunnoose":
>
>>
>>> I don't think we have missed anything - I have tested your code with
>>> several gcc compilers (new and old msp430 compilers, several AVR
>>> compilers, and gcc for the ColdFire - I have a batch file setup that I
>>> use for this sort of testing). They all agree with our interpretation
>>> of the code.
>>>
>>> However, I suggest you check it with IAR. Either it /is/ a bug, or
>>> they know more about the interpretation of horrible C code than we do
>>> :-)
>>> Thanks very much David, I'll do that. I refactored the code into the
>> following easy to understand snippet and it exhibits the same problem.
>> -------
>> static int log_calls(int num_iters)
>> {
>> puts("\nlogging");
>> return (num_iters-1);
>> }
>>
>> void myfunc(void)
>> {
>> signed int remaining_calls = 1;
>> while(P1IN==0)
>> {
>> remaining_calls = log_calls(remaining_calls);
>> if (remaining_calls> 0 )
>> {
>> continue;
>> } else {
>> break;
>> }
>>
>> }
>> }
> I think there is something wrong with the definition of P1IN.
> Looks like "#define P1IN 0"
>
P1IN is a register - it should be defined in a chip-specific header as a
"volatile uint8_t", or equivalent.
mvh.,
David
> Maybe somewhere in your header files is a mistake that overwrites P1IN.
>
> M.
>
> "ratishpunnoose":
>
>>
>>> I don't think we have missed anything - I have tested your code with
>>> several gcc compilers (new and old msp430 compilers, several AVR
>>> compilers, and gcc for the ColdFire - I have a batch file setup that I
>>> use for this sort of testing). They all agree with our interpretation
>>> of the code.
>>>
>>> However, I suggest you check it with IAR. Either it /is/ a bug, or
>>> they know more about the interpretation of horrible C code than we do
>>> :-)
>>> Thanks very much David, I'll do that. I refactored the code into the
>> following easy to understand snippet and it exhibits the same problem.
>> -------
>> static int log_calls(int num_iters)
>> {
>> puts("\nlogging");
>> return (num_iters-1);
>> }
>>
>> void myfunc(void)
>> {
>> signed int remaining_calls = 1;
>> while(P1IN==0)
>> {
>> remaining_calls = log_calls(remaining_calls);
>> if (remaining_calls> 0 )
>> {
>> continue;
>> } else {
>> break;
>> }
>>
>> }
>> }
Reply by ●March 3, 20112011-03-03
On 03/03/2011 16:25, Matthias Weingart wrote:
> "ratishpunnoose":
>
>> while(P1IN==0)
>
> What will happen when you write it this way:
>
> while(0==P1IN)
>
> M.
>
There should be no difference in the generated code. Of course, since
it looks like this is a bug in the compiler, it could be that the bug is
only triggered with one format.
I think writing things like "while (0 == P1IN)" is bad coding - it reads
backwards, and is therefore less clear than the logical "while (P1IN =0)" formulation.
If your compiler doesn't warn you when you accidentally write "while
(P1IN = 0)", then you need to learn how to use your compiler properly
(for gcc, use the "-Wall" flag). If your compiler is incapable of
giving such useful warnings, then you need to either get a decent
compiler, or use some sort of lint program. Poor compiler warnings are
no excuse for writing illogical code.
Remember, MISRA rules are 50% code, 50% meaningless, and 50% bad. This
convention is one of the bad ones.
Please learn to use spaces correctly - they make code /much/ easier to
read. Far and away the most commonly accepted spacing convention for
this expression is "while (P1IN == 0)".
> "ratishpunnoose":
>
>> while(P1IN==0)
>
> What will happen when you write it this way:
>
> while(0==P1IN)
>
> M.
>
There should be no difference in the generated code. Of course, since
it looks like this is a bug in the compiler, it could be that the bug is
only triggered with one format.
I think writing things like "while (0 == P1IN)" is bad coding - it reads
backwards, and is therefore less clear than the logical "while (P1IN =0)" formulation.
If your compiler doesn't warn you when you accidentally write "while
(P1IN = 0)", then you need to learn how to use your compiler properly
(for gcc, use the "-Wall" flag). If your compiler is incapable of
giving such useful warnings, then you need to either get a decent
compiler, or use some sort of lint program. Poor compiler warnings are
no excuse for writing illogical code.
Remember, MISRA rules are 50% code, 50% meaningless, and 50% bad. This
convention is one of the bad ones.
Please learn to use spaces correctly - they make code /much/ easier to
read. Far and away the most commonly accepted spacing convention for
this expression is "while (P1IN == 0)".
Reply by ●March 3, 20112011-03-03
There is no difference in the generated code as David said.
This occurrence is very specific to checking a volatile
as part of the while condition.
It could be while(volatile_reg),
while(volatile_reg & 0x03),
while(volatile_reg == y) etc.
However, if the while condition is based on an ordinary variable instead of a volatile, the sequence is compiled correctly.
Ratish
--- In m..., David Brown wrote:
>
> On 03/03/2011 16:25, Matthias Weingart wrote:
> > "ratishpunnoose":
> >
> >> while(P1IN==0)
> >
> > What will happen when you write it this way:
> >
> > while(0==P1IN)
> >
> > M.
> > There should be no difference in the generated code. Of course, since
> it looks like this is a bug in the compiler, it could be that the bug is
> only triggered with one format.
This occurrence is very specific to checking a volatile
as part of the while condition.
It could be while(volatile_reg),
while(volatile_reg & 0x03),
while(volatile_reg == y) etc.
However, if the while condition is based on an ordinary variable instead of a volatile, the sequence is compiled correctly.
Ratish
--- In m..., David Brown wrote:
>
> On 03/03/2011 16:25, Matthias Weingart wrote:
> > "ratishpunnoose":
> >
> >> while(P1IN==0)
> >
> > What will happen when you write it this way:
> >
> > while(0==P1IN)
> >
> > M.
> > There should be no difference in the generated code. Of course, since
> it looks like this is a bug in the compiler, it could be that the bug is
> only triggered with one format.
Reply by ●March 4, 20112011-03-04
Hi,
> > I don't think we have missed anything - I have tested your code
> > with several gcc compilers (new and old msp430 compilers, several
> > AVR compilers, and gcc for the ColdFire - I have a batch file
> > setup that I use for this sort of testing). They all agree with
> > our interpretation of the code.
> >
> > However, I suggest you check it with IAR. Either it /is/ a bug,
> > or they know more about the interpretation of horrible C code
> > than we do :-)
> > Thanks very much David, I'll do that. I refactored the code into
> the following easy to understand snippet and it exhibits the same
> problem.
[snip]
I tried your original code and the refactored code using IAR (compiler version 5.10.1.50144) and I could never get it to generate the assembly with the call to puts() removed.
This was using a realease build, fully optimised for size with a normal DLIB, for an F149. What project settings did you have?
> > I don't think we have missed anything - I have tested your code
> > with several gcc compilers (new and old msp430 compilers, several
> > AVR compilers, and gcc for the ColdFire - I have a batch file
> > setup that I use for this sort of testing). They all agree with
> > our interpretation of the code.
> >
> > However, I suggest you check it with IAR. Either it /is/ a bug,
> > or they know more about the interpretation of horrible C code
> > than we do :-)
> > Thanks very much David, I'll do that. I refactored the code into
> the following easy to understand snippet and it exhibits the same
> problem.
[snip]
I tried your original code and the refactored code using IAR (compiler version 5.10.1.50144) and I could never get it to generate the assembly with the call to puts() removed.
This was using a realease build, fully optimised for size with a normal DLIB, for an F149. What project settings did you have?
Reply by ●March 4, 20112011-03-04
People from IAR are reading here too. They should be able to verify your
problem. I do not use IAR, so I can not...
I still in doubt that this is a compiler fault - because the fault you are
reporting would hurt anybody else using the IAR. "while (P1IN);" or similar
is a very common use of sfr's. Maybe it is a problem with the compiler
setup.
Are you really sure that P1IN is the P1IN-sfr and not just a macro?
Try to compile your code with the preprocessor only. The preprocessor will
only do a macro expansion and you get C code without macros as a result.
(If preprocessing is not possible with IAR then read here:
http://en.wikipedia.org/wiki/C_preprocessor and use the GNU CPP)
Are you sure you are using the include files from your "latest version IAR-
compiler"? Or do you use the include files from a different compiler?
(Make a error in the msp430xxx.h where P1IN is defined - if your project is
still compiling with no errors you are using the wrong include files).
M.
"ratishpunnoose" :
>
> There is no difference in the generated code as David said.
>
> This occurrence is very specific to checking a volatile
> as part of the while condition.
> It could be while(volatile_reg),
> while(volatile_reg & 0x03),
> while(volatile_reg == y) etc.
>
> However, if the while condition is based on an ordinary variable instead
> of a volatile, the sequence is compiled correctly.
>
> Ratish
>
> --- In m..., David Brown wrote:
>>
>> On 03/03/2011 16:25, Matthias Weingart wrote:
>> > "ratishpunnoose":
>> >
>> >> while(P1IN==0)
>> >
>> > What will happen when you write it this way:
>> >
>> > while(0==P1IN)
>> >
>> > M.
>> >
>>
>> There should be no difference in the generated code. Of course, since
>> it looks like this is a bug in the compiler, it could be that the bug
>> is only triggered with one format.
>>
>>
problem. I do not use IAR, so I can not...
I still in doubt that this is a compiler fault - because the fault you are
reporting would hurt anybody else using the IAR. "while (P1IN);" or similar
is a very common use of sfr's. Maybe it is a problem with the compiler
setup.
Are you really sure that P1IN is the P1IN-sfr and not just a macro?
Try to compile your code with the preprocessor only. The preprocessor will
only do a macro expansion and you get C code without macros as a result.
(If preprocessing is not possible with IAR then read here:
http://en.wikipedia.org/wiki/C_preprocessor and use the GNU CPP)
Are you sure you are using the include files from your "latest version IAR-
compiler"? Or do you use the include files from a different compiler?
(Make a error in the msp430xxx.h where P1IN is defined - if your project is
still compiling with no errors you are using the wrong include files).
M.
"ratishpunnoose" :
>
> There is no difference in the generated code as David said.
>
> This occurrence is very specific to checking a volatile
> as part of the while condition.
> It could be while(volatile_reg),
> while(volatile_reg & 0x03),
> while(volatile_reg == y) etc.
>
> However, if the while condition is based on an ordinary variable instead
> of a volatile, the sequence is compiled correctly.
>
> Ratish
>
> --- In m..., David Brown wrote:
>>
>> On 03/03/2011 16:25, Matthias Weingart wrote:
>> > "ratishpunnoose":
>> >
>> >> while(P1IN==0)
>> >
>> > What will happen when you write it this way:
>> >
>> > while(0==P1IN)
>> >
>> > M.
>> >
>>
>> There should be no difference in the generated code. Of course, since
>> it looks like this is a bug in the compiler, it could be that the bug
>> is only triggered with one format.
>>
>>
Reply by ●March 4, 20112011-03-04
Thanks David for the indoctrination. ;-)
I just want to give hints how to find the problem.
M.
David Brown :
> On 03/03/2011 16:25, Matthias Weingart wrote:
>> "ratishpunnoose":
>>
>>> while(P1IN==0)
>>
>> What will happen when you write it this way:
>>
>> while(0==P1IN)
>>
>> M.
>> There should be no difference in the generated code. Of course, since
> it looks like this is a bug in the compiler, it could be that the bug is
> only triggered with one format.
>
> I think writing things like "while (0 == P1IN)" is bad coding - it reads
> backwards, and is therefore less clear than the logical "while (P1IN => 0)" formulation.
>
> If your compiler doesn't warn you when you accidentally write "while
> (P1IN = 0)", then you need to learn how to use your compiler properly
> (for gcc, use the "-Wall" flag). If your compiler is incapable of
> giving such useful warnings, then you need to either get a decent
> compiler, or use some sort of lint program. Poor compiler warnings are
> no excuse for writing illogical code.
>
> Remember, MISRA rules are 50% code, 50% meaningless, and 50% bad. This
> convention is one of the bad ones.
>
>
> Please learn to use spaces correctly - they make code /much/ easier to
> read. Far and away the most commonly accepted spacing convention for
> this expression is "while (P1IN == 0)".
>
>
>
I just want to give hints how to find the problem.
M.
David Brown :
> On 03/03/2011 16:25, Matthias Weingart wrote:
>> "ratishpunnoose":
>>
>>> while(P1IN==0)
>>
>> What will happen when you write it this way:
>>
>> while(0==P1IN)
>>
>> M.
>> There should be no difference in the generated code. Of course, since
> it looks like this is a bug in the compiler, it could be that the bug is
> only triggered with one format.
>
> I think writing things like "while (0 == P1IN)" is bad coding - it reads
> backwards, and is therefore less clear than the logical "while (P1IN => 0)" formulation.
>
> If your compiler doesn't warn you when you accidentally write "while
> (P1IN = 0)", then you need to learn how to use your compiler properly
> (for gcc, use the "-Wall" flag). If your compiler is incapable of
> giving such useful warnings, then you need to either get a decent
> compiler, or use some sort of lint program. Poor compiler warnings are
> no excuse for writing illogical code.
>
> Remember, MISRA rules are 50% code, 50% meaningless, and 50% bad. This
> convention is one of the bad ones.
>
>
> Please learn to use spaces correctly - they make code /much/ easier to
> read. Far and away the most commonly accepted spacing convention for
> this expression is "while (P1IN == 0)".
>
>
>
Reply by ●March 4, 20112011-03-04
Hi,
> People from IAR are reading here too. They should be able to verify
> your problem. I do not use IAR, so I can not...
>
> I still in doubt that this is a compiler fault - because the fault you
> are reporting would hurt anybody else using the IAR. "while (P1IN);" or
> similar is a very common use of sfr's. Maybe it is a problem with the
> compiler setup.
I think this is a bug, and I believe there is a release note for the IAR
compiler that shows that this is fixed.
Using data flow the compiler is able to prove that the while is not iterated
if P1IN is zero:
Original:
static int log_calls(int num_iters)
{
puts("\nlogging");
return (num_iters-1);
}
void myfunc(void)
{
signed int remaining_calls = 1;
while (P1IN == 0 && (remaining_calls = log_calls(remaining_calls)) > 0)
;
}
We know that the first call is log_calls(1) by data flow analysis and that
log_calls will return zero by data flow analysis and as a side-effect will
print something.
Hence, this can be transformed from what we know to:
void myfunc(void)
{
if (P1IN == 0)
puts("\nlogging");
(void)P1IN; // as P1IN volatile and requires testing in the while
}
That is the effect of this function. No more, no less.
The question is now what "puts" does. If it's
void puts(const char *) {}
Then the IAR compiler is correct. However, from the description of this and
the analysis the OP has done and provided, it smells like a compiler issue.
-- Paul.
> People from IAR are reading here too. They should be able to verify
> your problem. I do not use IAR, so I can not...
>
> I still in doubt that this is a compiler fault - because the fault you
> are reporting would hurt anybody else using the IAR. "while (P1IN);" or
> similar is a very common use of sfr's. Maybe it is a problem with the
> compiler setup.
I think this is a bug, and I believe there is a release note for the IAR
compiler that shows that this is fixed.
Using data flow the compiler is able to prove that the while is not iterated
if P1IN is zero:
Original:
static int log_calls(int num_iters)
{
puts("\nlogging");
return (num_iters-1);
}
void myfunc(void)
{
signed int remaining_calls = 1;
while (P1IN == 0 && (remaining_calls = log_calls(remaining_calls)) > 0)
;
}
We know that the first call is log_calls(1) by data flow analysis and that
log_calls will return zero by data flow analysis and as a side-effect will
print something.
Hence, this can be transformed from what we know to:
void myfunc(void)
{
if (P1IN == 0)
puts("\nlogging");
(void)P1IN; // as P1IN volatile and requires testing in the while
}
That is the effect of this function. No more, no less.
The question is now what "puts" does. If it's
void puts(const char *) {}
Then the IAR compiler is correct. However, from the description of this and
the analysis the OP has done and provided, it smells like a compiler issue.
-- Paul.
Reply by ●March 4, 20112011-03-04
On 04/03/2011 10:41, Matthias Weingart wrote:
> Thanks David for the indoctrination. ;-)
> I just want to give hints how to find the problem.
>
Yes, I know that. I've just had to work with too much ugly code
recently, so took the opportunity for a rant.
Of course, what I wrote is still true :-)
mvh.,
David
> M.
>
> David Brown:
>
>> On 03/03/2011 16:25, Matthias Weingart wrote:
>>> "ratishpunnoose":
>>>
>>>> while(P1IN==0)
>>>
>>> What will happen when you write it this way:
>>>
>>> while(0==P1IN)
>>>
>>> M.
>>> There should be no difference in the generated code. Of course, since
>> it looks like this is a bug in the compiler, it could be that the bug is
>> only triggered with one format.
>>
>>
>>
>> I think writing things like "while (0 == P1IN)" is bad coding - it reads
>> backwards, and is therefore less clear than the logical "while (P1IN =>> 0)" formulation.
>>
>> If your compiler doesn't warn you when you accidentally write "while
>> (P1IN = 0)", then you need to learn how to use your compiler properly
>> (for gcc, use the "-Wall" flag). If your compiler is incapable of
>> giving such useful warnings, then you need to either get a decent
>> compiler, or use some sort of lint program. Poor compiler warnings are
>> no excuse for writing illogical code.
>>
>> Remember, MISRA rules are 50% code, 50% meaningless, and 50% bad. This
>> convention is one of the bad ones.
>>
>>
>>
>>
>> Please learn to use spaces correctly - they make code /much/ easier to
>> read. Far and away the most commonly accepted spacing convention for
>> this expression is "while (P1IN == 0)".
>>
>>
>>
> Thanks David for the indoctrination. ;-)
> I just want to give hints how to find the problem.
>
Yes, I know that. I've just had to work with too much ugly code
recently, so took the opportunity for a rant.
Of course, what I wrote is still true :-)
mvh.,
David
> M.
>
> David Brown:
>
>> On 03/03/2011 16:25, Matthias Weingart wrote:
>>> "ratishpunnoose":
>>>
>>>> while(P1IN==0)
>>>
>>> What will happen when you write it this way:
>>>
>>> while(0==P1IN)
>>>
>>> M.
>>> There should be no difference in the generated code. Of course, since
>> it looks like this is a bug in the compiler, it could be that the bug is
>> only triggered with one format.
>>
>>
>>
>> I think writing things like "while (0 == P1IN)" is bad coding - it reads
>> backwards, and is therefore less clear than the logical "while (P1IN =>> 0)" formulation.
>>
>> If your compiler doesn't warn you when you accidentally write "while
>> (P1IN = 0)", then you need to learn how to use your compiler properly
>> (for gcc, use the "-Wall" flag). If your compiler is incapable of
>> giving such useful warnings, then you need to either get a decent
>> compiler, or use some sort of lint program. Poor compiler warnings are
>> no excuse for writing illogical code.
>>
>> Remember, MISRA rules are 50% code, 50% meaningless, and 50% bad. This
>> convention is one of the bad ones.
>>
>>
>>
>>
>> Please learn to use spaces correctly - they make code /much/ easier to
>> read. Far and away the most commonly accepted spacing convention for
>> this expression is "while (P1IN == 0)".
>>
>>
>>
