Very strange comparison behavior

[DRW]

Alright, today I had the strangest coding situation ever.

Have a look at this:

Code: Select all

unsigned int MyVariable;

void VeryStrangeFunction(void)
{
    MyVariable = 259;

    if (MyVariable < 7)
    {
        Saveable.HeroEnergy = 473;
    }
}

We all agree that this condition should never be true?

Alright, but here it is, right in the top left corner of my screen:

Screenshot.png (3.18 KiB) Viewed 11650 times

This is what cc65 created out of it:

Code: Select all

.segment	"CODE"

.proc	_VeryStrangeFunction: near

.segment	"CODE"

	ldx     #$01
	lda     #$03
	sta     _MyVariable
	stx     _MyVariable+1
	bcs     L0681
	lda     #$D9
	sta     _Saveable+13
	stx     _Saveable+13+1
L0681:	rts

.endproc

Would you say this is correct code? I mean, where is the number 7 here? Where is the actual comparison?
If I change the code to MyVariable < 100, it's still the same.

O.k., maybe the compiler already optimizes the comparison since the variable always has the same value. But then, how is it possible that the HeroEnergy variable is ever set by this function?

Also, please note that the function does and doesn't produce the strange behavior, dependent on where I call it. I haven't really found out the reason, but if I have code where I do several things and then I call this function here, the question whether the hero's energy is changed is dependent on where I call the function. Sometimes it changes the energy, sometimes it doesn't.

Also, when I assign a value less than 256 to the variable, the compiler immediately optimizes the condition away:

Code: Select all

void VeryStrangeFunction(void)
{
	MyVariable = 250;

	if (MyVariable < 7)
	{
		Saveable.HeroEnergy = 473;
	}
}

Code: Select all

.segment	"CODE"

.proc	_VeryStrangeFunction: near

.segment	"CODE"

	ldx     #$00
	lda     #$FA
	sta     _MyVariable
	stx     _MyVariable+1
	rts

.endproc

But for values from 256 and larger, it doesn't do the complete optimization, even though the condition result should be the same.

Does anybody have any idea what's going on here?

[rainwarrior]

I'd say it's just a compiler bug. There's nothing in that generated code to affect carry before it's used. The comparison is missing.

Maybe something to do with optimization vs. 16-bit variables? I dunno. If you can make a minimum example I'm sure the CC65 project would appreciate an issue report on the github. (Actually, I'll report it, I already made the minimal example to test it for myself, and it's a repeatable problem.)

[dougeff]

Yeah. Wrong code. Weird.

What are your optimization settings on the compiler? -Oirs

[DRW]

(Actually, I'll report it, I already made the minimal example to test it for myself, and it's a repeatable problem.)

Alright, that's good.

What are your optimization settings on the compiler? -Oirs

Simply -O, but -Oirs works as well.

In the meantime:
Can anybody please tell me how a proper comparison of an unsigned int variable with a constant value (that is also in the range of unsigned int) has to look like in assembly, so that I can write it in inline assembly?
(I still have problems wrapping my head around all those carry flags etc.)

[rainwarrior]

Reported it here:
https://github.com/cc65/cc65/issues/895

[rainwarrior]

In the meantime:
Can anybody please tell me how a proper comparison of an unsigned int variable with a constant value (that is also in the range of unsigned int) has to look like in assembly, so that I can write it in inline assembly?
(I still have problems wrapping my head around all those carry flags etc.)

I recommend this article for reference on that, I refer to it whenever I forget:
http://6502.org/tutorials/compare_beyond.html#4.3

But the quick version:

Code: Select all

lda a+0     ; low byte first
cmp b+0     ; CMP is equivalent to SEC, SBC
lda a+1     ; high byte second
sbc b+1
; carry is now (a >= b)

[DRW]

Thanks. I'll have a look at it.

[Banshaku]

I had strange bugs in my C code when doing 8 bits operations on 16 bits values but I wasn't aware that it was the compiler. There was other issues too but I would need to find back what they were. Good to see that there was a reason for that madness

edit: I may have forced the value to 16 bits like 0x0007 to make it work but I don't have a good recollection of it so for now it may not fix your issue. I would need to test it first, which I can't at the moment.

[rainwarrior]

I don't think this is a common bug, Banshaku. I've written lots of CC65 code with 16-bit variables that works fine. They have regression tests for a ton of this stuff too. This compiler is obviously not as bulletproof as a commercial compiler, but it's definitely had a lot of people kicking the tires.

I believe this is specifically to do with a dead code removal optimization, probably only affects a very rare case which DRW stumbled on, but I dunno, we'll see what is found with this bug.

[Banshaku]

I'm not saying that it's full of bug, this was not my intention and the compiler works fine and have a lot of code in C in my current project, but more that I had one~two strange bugs similar to that before and it was occurring while checking values in a similar way. It seemed to affect value with different width (8/16 bits) but my memory is quite vague about it. So I may have encounter it before and was sure I was just doing something wrong since I always blame myself first, then myself again before blaming the tool. Now that this was found, is this behavior arise again in my code, I will be able to confirm if it was this case or not, which is great

[DRW]

By the way: My original problem happened when the variable was not assigned to a fixed value at compile time. So, there seems to be an issue even if the compiler cannot optimize the conditional code path away.
I'll have to check again whether this is an unrelated issue or whether this is still something similar. (I can do this in about 12 hours again.)

My original code was something like this:

Code: Select all

#define FirstScreenIdBank1 0
#define FirstScreenIdBank2 7
#define FirstScreenIdBank3 12
#define FirstScreenIdBank4 300

#define Bank1Number 0
#define Bank2Number 1
#define Bank3Number 2
#define Bank4Number 3

unsigned int Temp;

unsigned char __fastcall__ GetScreenBankNumber(void)
{
    if (Temp < FirstScreenIdBank2)
        return Bank1Number;

    if (Temp < FirstScreenIdBank3)
        return Bank2Number;

    if (Temp < FirstScreenIdBank4)
        return Bank3Number;

    return Bank4Number;
}

#define AssignScreenBankNumber(variable, screenId)\
{\
    Temp = screenId;\
    variable = GetScreenBankNumber();\
}

unsigned int CurrentScreenId;
unsigned char CurrentScreenBankNumber;

void __fastcall__ Buildup(void)
{
    /* Some stuff */
    
    AssignScreenBankNumber(CurrentScreenBankNumber, CurrentScreenId);
    
    /* Other stuff */
}

It worked fine as long as the screen ID is 255 or lower. Yesterday was the first time when I had a value of more than 255.

So, yeah, originally, it was not such an artificial situation.
As I said, I'll tell you later whether this kind of function still produces the phenomenon or whether my original problem of the game crashing is still at another place.

[supercat]

Can anybody please tell me how a proper comparison of an unsigned int variable with a constant value (that is also in the range of unsigned int) has to look like in assembly, so that I can write it in inline assembly?

There are a number of optimal variations for different constant values. For (constant) values of n less than 65535, `i<=n` may sometimes be more cheaply be replaced with `i<(n+1)`, and for non-zero n, `i<n` may sometimes be replaced with `i<=(n-1)`.

For the general case, the pattern:

Code: Select all

    lda lo
    cmp #<value
    lda hi
    sbc #value

will clear the carry if `hi:lo` is less than value; the state of the zero flag isn't meaningful. One can swap the roles of the operands or adjust `value` as indicated above if it is necessary to test other conditions. If the constant is less than 256 (or can be adjusted to 255), the pattern:

Code: Select all

    lda hi
    bne notLess
    lda lo
    cmp #value

will [if the bne target is in range] take the same amount of code and have the same worst-case time, but execute more quickly in some cases. For comparisons of the form x<$xx00 or x<=$xxFF, one can ignore the low-order byte of `x` and process the high-order byte directly.

There are many cases that a good compiler should optimize for, and cc65 wouldn't be the first compiler that gets some wrong.

[DRW]

Alright, I checked my actual code again. Because after I was looking for the issue for such a long time yesterday, I created that minimalistic example that I presented to you.
But in actuality, I of course don't have a situation where I compare a variable with a fixed value after I just set it with a fixed value.

Anyway, it turns out that my actual original problem was caused by the fact that I was still using the classic cc65 compiler from http://www.cc65.org.
(This is the compiler with which I started programming the NES and it's the one that "City Trouble" was written in.)

But in the current case, the output between the old and the new compiler is indeed different and using the new assembly code doesn't produce the error anymore.

(However, the example from yesterday is still a bug in both versions of the compiler.)


So, it looks like it's time to convert my code to the new cc65 after all.

[rainwarrior]

For the time being, the specific bug you ran into can be avoided by disabling the broken optimization that caused it. That can be done from the cc65 command line with:

--disable-opt OptCmp8

As expected this bug is very limited in scope. It only affects comparing two 16 or 32 bit values that are detected as constant at compile time, and only 50% of the time (depending on prior state of carry). I'm working on a pull request to fix it.


Also, as an aside, there is a command line flag: --debug-opt-output

This will output, for each function, a function.opt file that shows every optimization step applied in series. You can see exactly how it optimizes, and try to identify which specific step was causing the problem.

[DRW]

Thanks for the information.

However, my sample code was a very contrived example. It's not actually something that's in my game. I just posted it here because it was the easiest way to demonstrate the strange behavior.

My actual code is similar to the one four posts above. And this already was corrected in the new cc65, so switching the compilers was enough to fix it.

It was a pure coincidence that my artificial "known values at compile time" example turned out to have a similar bug that was not yet corrected.

Besides, the bug that I originally encountered with the old cc65 was a similar case as the one you described in github: The compiler was omitting a CMP #0 before a comparison, even though it shouldn't. So, a similar issue was already detected and fixed by the developers of the new cc65 at some point in the past.