Why does the compiler insert this instruction? [CPC]

Go To Last Post
13 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
					scpi_result_t result = SCPI_RES_ERR;
                        if ((result = context->cmdlist[i].callback(context)) == SCPI_RES_OK)
						{
                        }
						else if (!context->cmd_error)
						{
                            SCPI_ErrorPush(context, SCPI_ERROR_EXECUTION_ERROR);
						}
                        if ((result = context->cmdlist[i].callback(context)) == SCPI_RES_OK)
00000B3B  LD R0,Z+		Load indirect and postincrement 
00000B3C  LDD R31,Z+0		Load indirect with displacement 
00000B3D  MOV R30,R0		Copy register 
00000B3E  ADD R30,R4		Add without carry 
00000B3F  ADC R31,R5		Add with carry 
00000B40  LDD R0,Z+2		Load indirect with displacement 
00000B41  LDD R31,Z+3		Load indirect with displacement 
00000B42  MOV R30,R0		Copy register 
00000B43  MOVW R24,R14		Copy register pair 
00000B44  ICALL 		Indirect call to (Z) 
00000B45  CPI R24,0x01		Compare with immediate 
00000B46  CPC R25,R1		Compare with carry 
00000B47  BREQ PC+0x0C		Branch if equal 
						else if (!context->cmd_error)
00000B48  MOVW R26,R14		Copy register pair 
00000B49  ADIW R26,0x14		Add immediate to word 
00000B4A  LD R24,X		Load indirect 
00000B4B  SBIW R26,0x14		Subtract immediate from word 
00000B4C  TST R24		Test for Zero or Minus 
00000B4D  BRNE PC+0x06		Branch if not equal 
                            SCPI_ErrorPush(context, SCPI_ERROR_EXECUTION_ERROR);
00000B4E  MOVW R24,R14		Copy register pair 
00000B4F  LDI R22,0x38		Load immediate 
00000B50  SER R23		Set Register 
00000B51  CALL 0x0000012A		Call subroutine 

After the ICALL (call to my implemented callback which always returns 0x01 (verified by inspection of R24)), there is the comparison conditional on the return value of my callback.

The Z flag is set by CPI and cleared by CPC -- I do not see why R25 and R1 are relevant to my desired code. Originally the code was

if (!callbackReturn && !context->cmd_error) { pushErrorOnStack() }

but I have broken it up in attempts to get the assembler to not generate the CPC instruction.

What's going on guys? I have no theory :-\

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

For what it is worth,

if ((result = context->cmdlist[i].callback(context)) == SCPI_RES_ERR)
{
	if (!context->cmd_error)
	{
		SCPI_ErrorPush(context, SCPI_ERROR_EXECUTION_ERROR);
	}
}

produces what I believe is the desired operation. If the callback returned an error and there is not already an error (context->cmd_error), then push an error.

The new assembly is:

                        if ((result = context->cmdlist[i].callback(context)) == SCPI_RES_ERR)
00000B3B  LD R0,Z+		Load indirect and postincrement 
00000B3C  LDD R31,Z+0		Load indirect with displacement 
00000B3D  MOV R30,R0		Copy register 
00000B3E  ADD R30,R4		Add without carry 
00000B3F  ADC R31,R5		Add with carry 
00000B40  LDD R0,Z+2		Load indirect with displacement 
00000B41  LDD R31,Z+3		Load indirect with displacement 
00000B42  MOV R30,R0		Copy register 
00000B43  MOVW R24,R14		Copy register pair 
00000B44  ICALL 		Indirect call to (Z) 
--- C:\Users\sag\Documents\SVN\UVAPD\Firmware\Debug/.././parser.c --------------
00000B45  SER R31		Set Register 
00000B46  CPI R24,0xFF		Compare with immediate 
00000B47  CPC R25,R31		Compare with carry 
00000B48  BRNE PC+0x0C		Branch if not equal 

At this time, the brne is taken like I wanted.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

first guess :
you use a int not a char
and sometimes the compiler can't see if it's only to do 8 bit calculations (C say that calculations on char defaults to int).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

produces what I believe is the desired operation.

But that also has a CPC! I thought that was what you were railing against?

Quote:

The Z flag is set by CPI and cleared by CPC -- I do not see why R25 and R1 are relevant to my desired code.

As mentioned, from the fragment we cannot see the type/size of the returned value, or "result". Also as mentioned by the rules of C "things" are often widened to int width.

R1 is the zero register. R25 is the high byte of the return value. The GCC gurus will correct me on details, but the code was produced according to the rules of C, and build options, and the phase of the moon, or whatnot. (There have been some extensive threads on how to coerce 8-bit operations on 8-bit pieces.)

While you probably have to live with it, that's beside the point somewhat. Does the code sequence shown properly do a compare of "result" against 0x01? From the venerable AVR202 app note on "16-Bit Arithmetics":

Quote:
Compare Two 16-bit Register Variables

This operation is done as follows:
1. Compare Low bytes.
2. Compare with carry High bytes.

Note that the Compare with Carry instruction supports zero-propagation, which means that all conditional branch instructions can be used following the two-step compare operation. By adding more Compare with Carry instructions, numbers of n-byte width can be compared using n instructions.

Compare a 16-bit Register with a 16-bit Immediate

This operation is done as follows:
1. Compare register Low byte to immediate Low byte.
2. Store immediate High byte to a third register.
3. Compare with carry High bytes.

The compiler-generated sequence shown is a hybrid of the two, with an immediate compare for the first (low) byte and using the zero-register R1 for the high byte.

From the AVR Instruction Set document description of CPC I see

Quote:
Z: R7 •R6• R5• R4 •R3 •R2 •R1• R0 •Z
Previous value remains unchanged when the result is zero; cleared otherwise.
[note: It didn't paste well; that is /R7, /R6, etc.]

The result of comparing the high byte R25 and the zero-register R1 should indeed be zero if R25 is the extension of 0x01.

Now comes the interesting part: Either R25 or R1 is not zero. How can that happen?

[edit] If it was me digging further, I'd look at the code for the tail of the invoked routine to see if the return value is placed in R24 >>and widened to include<< R25. From the fragment shown we can see R24/R25 used for the function parameter.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:
Now comes the interesting part: Either R25 or R1 is not zero. How can that happen?
There was another post recently with a similar complaint. The root cause there was that the function being called was actually returning an 8-bit value but the compiler was assuming (because of the absence of a declaration) that the function returned an int value. The code shown by the OP in this thread would be consistent with that set of circumstances.

Don Kinzer
ZBasic Microcontrollers
http://www.zbasic.net

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Sometimes comparison code might be confusing and you'll have to understand how the instructions are interworking, e.g. for

void g (int i)
{
    void f (void);
    if (i < 2000 && i > 13)
        f();
}

the compiler generates

g:
	sbiw r24,14
	cpi r24,-62
	sbci r25,7
	brsh .L1
	rjmp f
.L1:
	ret

which does exactly what the C code requires.

In your case, there are too much guesses. We don't know the prototypes of the function, we don't know the compiler version you are using, we don't know the variable types, we don't know the compiler options, we don't know the context of the snippet, we dopn't know if your base version comes with patches, we don't know how your compiler is configured, ...

For example, some time ago there was a silent ABI change (integral 8-bit return values are no more promoted to 16 bits), the ABI specifies that char is signed bit many users change the ABI so that it is unsigned, etc.

To reduce confusion with prototypes you can compile with -Werror=strict-prototypes -Werror=missing-prototypes.

avrfreaks does not support Opera. Profile inactive.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you folks for the wonderful discussion, I am currently digesting a good portion of it. In working with this issue, I expanded and modified the conditional to be less compact. In addition, I will show you all some prototypes and enumerations used with the faulty code.

types.h

    typedef enum _scpi_result_t scpi_result_t;
    enum _scpi_result_t {
        SCPI_RES_OK = 1,
        SCPI_RES_ERR = -1,
    };
    typedef scpi_result_t(*scpi_command_callback_t)(scpi_t *);

The callback is of type scpi_command_callback_t and has prototype

scpi_result_t SCPI_FrequencyArmStopTime(scpi_t * context) { ... }

I am in contact with the author of this library, j123b567. His project page is located @ https://github.com/j123b567/scpi-parser
I am going to do a fresh pull of the code and note every step I take with it to port to my avr studio 6 environment. This will allow me to pin down the exact step that fixes the issue.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

The Z flag is set by CPI and cleared by CPC -- I do not see why R25 and R1 are relevant to my desired code. Originally the code was

It's trying to differentiate between the function having returned 1 (0x0001) or -1 (0xFFFF) in 16 bits.

Why not make the return type uint8_t with 0 and 1 as allowable values? Far better than using a signed return in a wide type.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Far better than using a signed return in a wide type.

Indeed that should save a few flash words and cycles.

However, back to the symptoms described in the first post(s). I seem to recall some discussions about the sizeof an enum. I'd think it would be 16 bits because of the -1? So I'm back to asking to see the tail of the offending callback function. Perhaps there was a cast so that the full 16-bit value wasn't loaded into R24/R25? With the further information about the typing the original source code looks OK.

That said, what is the type of "result"? (And from the fragment shown it might not matter--I don't see any assignment code so the compiler is just keeping it in register(s)?)

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But I thought OP was simply asking why R25,R1 were involved? Surely that's explained by it simply being a signed 16 bit test? It compares R24 with 0x01 and R25 with 0 (always held in R1) incorporating the carry.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson is right in the sense that the 16-bit compare was an issue in my code. Upgrading to a later version of the library, a few elements have changed.

I am still using the signed enum (I have also substituted a uint with 0 and 1 as values) since it works. The main issue I had was with the included types.h file since the compiler it was written for apparently was fine with 'forward declared?' typedef enums.

Arranging the library's types to GCC's satisfaction, I can say that the problem is solved

    if (cmd->callback != NULL) {
        if ((context->paramlist.cmd->callback(context) != SCPI_RES_OK) && !context->cmd_error) {
     f48:	c3 01       	movw	r24, r6
     f4a:	09 95       	icall
     f4c:	81 30       	cpi	r24, 0x01	; 1
     f4e:	49 f0       	breq	.+18     	; 0xf62 
     f50:	f3 01       	movw	r30, r6
     f52:	84 89       	ldd	r24, Z+20	; 0x14

and the day is saved. I appreciate the missing and strict prototype flags as well as the superb assistance.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

But I thought OP was simply asking why R25,R1 were involved?

Not exactly.
Quote:

The Z flag is set by CPI and cleared by CPC ...

.. and then the quest to eliminate the CPC started.
Quote:

After the ICALL (call to my implemented callback which always returns 0x01 (verified by inspection of R24)), there is the comparison conditional on the return value of my callback.

So if the function returns 0x01, and R25 and R1 are indeed zero, then the CPC should not be changing the Z flag.

Which is why I kept asking for a code dump from the callback routine. As R25 is a parameter byte which could very well be non-zero, why wasn't the returned value 16 valid bits?

As the code is now re-written to avoid the situation, we'll probably never know.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Older compiler versions promote 8-bit integral return types to 16 bits. Without code and compiler version and options you can just guess.

avrfreaks does not support Opera. Profile inactive.