Thursday, September 26, 2013

Moving on, there is a missing typecast

unsigned char Round;
#define VDPWD *((volatile char*)0x8C00)
void modulo()
{
  VDPWD = '0' + Round % 10;
}

This compiles to:

modulo
    movb @Round, r1    * get the variable into R1 MSB
    srl  r1, 8         * move to LSB
    mov  r1, r2        * copy to R2
    clr  r1            * zero R1 (32-bit value is now >000000xx)
    li   r3, >A        * load R3 with 10 (divisor)
    div  r3, r1        * do the division (R1 has dividend, R2 has remainder)
    mov  r2, r1        * copy R2 to R1 - however, remainder was 16 bit! We are about to treat it like 8-bit.
* Missing "swpb r1" here
    ai   r1, >3000     * add '0' as an 8-bit byte value - this is wrong <--- br="">    movb r1, @>8C00    * move the result to the video chip

Looking through the RTL, we find this sequence in 131r.initvals:

(insn 9 8 10 3 tursi5.c:6 (set (reg:QI 21 [ D.1197 ])
        (plus:QI (subreg:QI (reg:HI 25) 1)
            (const_int 48 [0x30]))) -1 (nil))

In assembly, this would be:

swpb r1  * <-- ah="" br="" ha="" instruction="" is="" missing="" our="" this="">ai r1, >3000

Somewhere in step 172, register allocation, the type conversion gets lost

(insn 9 8 11 2 tursi5.c:6 (set (reg:QI 1 r1 [orig:21 D.1197 ] [21])
        (plus:QI (reg:QI 1 r1 [orig:21 D.1197 ] [21])
            (const_int 48 [0x30]))) 59 {addqi3} (nil))

In assembly, this would just be:

ai r1, >3000

Poop, we've found yet another case where GCC assumes that no effort is required to switch between 8-bit and 16-bit values. In order to fix this, we're going to have to dig deep into the guts.

Wednesday, September 25, 2013

I've been busy getting up to speed with a new job lately, but I finally got some time to do some TI stuff.

While I was away, Tursi found a few bug in GCC that need to be addressed. These range from simple fixes to potentially long and painful sessions tracking down where a bytewise operation gets broken. Most of these were submitted by Tursi on the AtariAge forum, so thank you for finding these.

Let's get cracking!

First up is an easy fix for jeq/jlt and jeq/jgt pairs. There are no instructions to express "jump if arithmetically greater/lesser or equal to" like there are for logical comparisons. Those would be JLE and JHE. Instead, we must chain two conditional jumps together to get the same result. The conditional flags are not affected by any of the jump instructions, so we won't get weird behavior by doing this. I made an effort to encourage GCC to not use these combinations when possible, but sometimes there's just no avoiding the situation.

For simplicity's sake, I'll only refer to the jeq/jlt pair here. The jeq/jgt pair has the same issue, just with a different test.

I originally had arbitrarily chosen to test for equality first then for a lesser value. Everything worked, so I moved on to other issues and didn't think about it. However Tursi recommended doing the lesser test first. The idea here is that the less-then test will match a larger set of possible conditions than the equality test, so by doing that one first, the second test may not be needed and performance is increased.

Next up, we're missing an ANDI instruction in the compilation of this snippet:

extern int byte2hex[];
void fast_hex_out(int x) {
        unsigned char z=(x&0xff);
        unsigned int dat = byte2hex[z];
        VDPWD = dat>>8;
        VDPWD = dat&0xff;
}

This compiles to:

fast_hex_out
* Should be "andi r1, >00FF" here
        a    r1, r1
        mov  @byte2hex(r1), r2
        mov  r2, r3
        li   r1, >8C00
        movb r3, *r1
        swpb r2
        movb r2, *r1
        b    *r11

So, after digging into the RTL, we see that at one point, GCC intended to put that instruction there, but it got stripped out.

From step 182r.peephole2:
(insn 38 37 10 2 tursi4.c:15 (and (reg:HI 1 r1 [30])
        (const_int 255 [0xff])) -1 (nil))

Wait a second... THIS INSTRUCTION DOES NOT SET ANYTHING! IT'S TOTALLY USELESS! Of course it's being deleted.
This is the result of a bad peephole. GCC would never do anything this dumb on its own. Backing up a bit, we find the pre-peephole instructions:

From 172r.ira:
(insn 7 4 9 2 tursi4.c:15 (set (reg:QI 1 r1 [orig:27 x+1 ] [27])
        (subreg:QI (reg:HI 1 r1) 1)) 73 {movqi} (nil))

(insn 9 7 10 2 tursi4.c:15 (set (reg:HI 1 r1 [30])
        (zero_extend:HI (reg:QI 1 r1 [orig:27 x+1 ] [27]))) 75 {zero_extendqihi2} (nil))

This translates to:
   (char)r1 = (char)((int)r1)
   (int)r1 = (int)((char)r1)

With all this in mind, I found the broken peephole optimization. When I was looking at it, the problem was obvious. I feel really silly right now.

Thursday, May 2, 2013

GCC 4.4.0 patch 1.8

Changes this version:

Fixed R11 restoration in epilogue being dropped by DCE
Added support for named sections
Removed support for zeroing memory bytes, was buggy with some devices

Download:
gcc-4.4.0-tms9900-1.8-patch.tar.gz

Binutils 2.19.1 patch 1.5

Changes this version:

Added more informative syntax error messages
Fixed values like ">6000" in strings being mangled
Confirm support for named sections

Download:
binutils-2.19.1-tms9900-1.5-patch.tar.gz

Sunday, April 28, 2013

I was looking into the problem Tursi reported where the link register is not properly restored. The issue here is that a few earlier improvements are combining to cause this. An earlier change was to merge modifications of the stack pointer in an effort to improve performance. Another one was to make R11 be a volatile register, whose value will be destroyed after a function call. The problem here is that since the example program uses stack space, but only needs to restore R11, an instruction like "mov *r10, r11" was issued. The dead code elimination pass saw this as a useless instruction, since R11 seemed to have no further use, and the stack pointer in R10 was not changed.

Here's what I'm talking about:

          ...
        bl   @puts
        mov  *r10, r11  # Instruction deemed unnecessary and deleted by DCE pass
        ai   r10, >6
        b    *r11

The fix here was to modify the invocation of the UNSPEC_RETURN instruction to use R11 as an input. This causes the dead code elimination pass to see that "mov *r10, r11" serves a useful purpose and is not to be removed. One bug down...

In other news, I've confirmed that the -ffunction-sections and -fdata-sections options work, as well as the section attribute. All this confirms that named section suppport is working as intended. The elf2card and elf2ea5 tools won't do anything with them, but I assume that linker files will be used to combine them into the standard .text .data and .bss sections.

The final bug is clearing a memory byte. Tursi is working on hardware which is uses write-only registers. The problem for him is that code like:
    *(volatile char*)0x9c02 = 0

results in an instruction like:
    sb @>9c02, @9c02

If the destination lies in RAM, this is fine. But write-only hardware could potentially return different values for each of the two memory fetches. In this case, it would be more appropriate to use something like this:
    clr r0
    movb r0, @>9c02

This is the same number of code bytes (6), and only a few cycles slower. Not too bad. Fixing this is really easy too. There was a mode for that instruction to specifically permit directly setting memory to zero. By removing that permission, everything works as intended.

Sunday, April 14, 2013

I've been away from TI stuff for a long time, but I can some work done today.

On AtariAge, Tursi noted that strings like ">6000" were mysteriously being converted to "$6000". That was due to fairly dumb text replacement code in GAS. Since GAS doesn't understand hex constants like >6000, and ">" is treated as a logical operator, I needed to transform the incoming text before it gets parsed for assembly. Unfortunately, I didn't make sure these constants in text strings were not affected.

So that's now fixed.

I started looking into named sections, and it turns out that I don't really need to do any work here. GAS already has support for ".section" and the GCC support just had to be uncommented in the config file. Handy!

Friday, February 15, 2013

OK, the changes to GAS parsing is done. I think this should make it easier to find problems. So here's a test file with lots of errors:

    eric@compaq:~/dev/tios/src/gas_test$ cat gastest.asm
        mov r1
        mov r1+,r2
        mov r1,r2+
        inv r1, r2
        inv r

And here's the result from the new parsing code:

    eric@compaq:~/dev/tios/src/gas_test$ tms9900-gas gastest.asm
    gastest.asm: Assembler messages:
    gastest.asm:1: Error: unexpected end of line
    gastest.asm:2: Error: unexpected character at "+,r2"
    gastest.asm:3: Error: unexpected character at "+"
    gastest.asm:4: Error: unexpected character at ",r2"
    Invalid register starting at "r"
    gastest.asm:5: Error: missing argument

Could this be better? Maybe, but it's a lot better than before. Time to get back to floating point code.