Problems implementing the SBC instruction

Problems implementing the SBC instruction
by NESmongoose on 2008-04-10 (#32549)

Hi.

I'm currently debugging my CPU using the nestest ROM.

According to the ROM output, all addressing modes have issue with the SBC instruction.

However, I just cannot see what's wrong with it, having spent about 2 hours rewriting and rewriting it.

I'd appreciate it if anybody could give me some pointers on where it may be going wrong.

Thank you.

Code:

carry = ~P & 0x1;
      
// Perform the SBC.
newA = A - operand - carry;
      
// if the sign bit is incorrect, set the overflow flag.
if ((~(A ^ operand) & (A ^ operand) & 0x80) != 0) {
   P |= 0x40;
}
      
// if overflow occurs, clear the carry flag.
if (newA >= 0) {
   P |= 0x1;
}
      
A = newA & 0xFF;
   
// The zero and negative flags.
if (A == 0) {
   P |= 0x2;
}
if ((A & 0x80) == 1) {
   P |= 0x80;
}

by mic_ on 2008-04-10 (#32551)

I just implemented it as ADC(operand^0xFF).

by Disch on 2008-04-10 (#32553)

1) You're checking overflow wrong. Overflow logic is: "positive - negative = negative" or "negative - positive = positive". I don't know what you're checking for now -- but it effectively looks like you'll always get zero, since you're ANDing A^operand with its compliment (which will always give you zero).

for pos - neg = neg ... 'A' is the odd one out, meaning its sign must not match either 'newA' or 'operand'. So you'd want something like the following:

Code:
if((A ^ operand) & (A ^ newA) & 0x80) SetV();

2) You're not clearing flags! You're only setting them! N, V, C, and Z all get cleared if you don't set them. Put in elses or something to clear them where appropriate... or clear them all before you do the checks to set them.

3) You're never setting N. "if ((A & 0x80) == 1)" will always be false, since A&0x80 will always be either 0 or 0x80 (never 1). "if(A&0x80)" works just fine... no need to do any == check.

4) "ADC(operand^0xFF)" will work just as mic_ suggested.. however if you have the above problems with SBC, I'm assuming you have similar problems with ADC.

by blargg on 2008-04-10 (#32562)

Quote:
You're not clearing flags! You're only setting them! N, V, C, and Z all get cleared if you don't set them.

Too bad the term "set" can mean it either replaces the flag or sets it to 1. Probably better to say that ADC replaces or overwrites these flags.

Quote:
"if ((A & 0x80) == 1)" will always be false

The rule-of-thumb in C is to always compare with zero (false), never 1 (true), since in many contexts non-zero values other than 1 are also considered true. So if ( n != 0 ) or if ( n == 0 ), and with the implied != 0 for conditionals, you can shorten it to if ( n ) and if ( !n ) (even with pointers).

by WedNESday on 2008-04-10 (#32568)

WedNESday's code:

Code:
CPU.TMP = 1 - CPU.CF;

CPU.TMP2 = (char)CPU.A - (char)CPU.Databus - CPU.TMP;

if( CPU.TMP2 < -128 || CPU.TMP2 > 127 )
CPU.OF = 0x40; else CPU.OF = 0x00;

CPU.NF = CPU.ZF = CPU.A = CPU.CF = CPU.A - CPU.Databus - CPU.TMP;

if( CPU.CF > -1 )
CPU.CF = 1; else CPU.CF = 0;

Probably the trickiest opcode, what with most people not understanding what overflow is etc.

by Zepper on 2008-04-10 (#32569)

If "unsigned", you should AND 80h instead of a double-condition.

by blargg on 2008-04-10 (#32577)

Might as well link to this when it was discussed a few weeks ago (ADC, where SBC is just ADC operand^0xFF): "Got any tips for Early NES Emulator Development?"

by NESmongoose on 2008-04-12 (#32628)

Thank you all!

My CPU now passes all of nestests' tests.

Cheers