GCC for the TI

[+khanivore]

D’oh, good catch, thanks.  I refactored the reg saving code to only save R11 if other functions were called by the function but never thought of the scenario where it may be declared clobbered by an inline.  If it is saving the stack reg then I do need to declare R15 as fixed.  I had mistakenly assumed the compiler didn’t need to be told that and wouldn’t be stupid enough to try and save the stack pointer to the stack but clearly it is.  Will spin a new patch tomorrow.

[+khanivore]

I've pushed patch 1.26 to fix those regressions.  I've tested using inline asm that clobbers r11 and the compiler does emit a save of r11.  It also no longer emits a save of r15.  @chue I think this was causing the size increase you saw.

I noticed that when compiling a function that takes a variable parameter list (e.g. printf (char *fmt, ...)), the variable params AND the parameter immediately preceding the variable list are pushed to the stack and not passed through regs.  It is possible to modify this to pass all non-variable params as registers, but I'll mark that as a future change for now.

 

Debian package also attached.

 

Edited December 18, 2023 by khanivore
deleted out-dated package

[Tursi]

My testing here seems to pass. All my test apps are slightly bugged so that's not a promise everything's solid, but definitely all the regressions I saw are solved and the raw code in Example1 looks pretty good. 

 

My vgmcomp2 test app works, but doesn't display the text strings correctly. Right now, I'm not going to dig into that cause I'm not 100% sure it was working right before. Standalone test apps didn't show any issues.

 

I just need to decide how I'm gonna handle the the binary change in my libs... I think I might just push a new branch for the older compiler. There's not a lot that relies on it, but there's some.

 

 

[+khanivore]

Good stuff.  If it's just the stack pointer that may cause binary backward compatibility issues I can spin a version where we go back to R10 as SP and make R15 a general reg.  At the end of the day, the register numbers are arbitrary.  It might give a tiny advantage to have them as contiguous as possible for 32-bit ops but very marginal I'd say.  Maybe for consistency, I should stick with R10 ...

[Tursi]

1 hour ago, khanivore said:

Good stuff.  If it's just the stack pointer that may cause binary backward compatibility issues I can spin a version where we go back to R10 as SP and make R15 a general reg.  At the end of the day, the register numbers are arbitrary.  It might give a tiny advantage to have them as contiguous as possible for 32-bit ops but very marginal I'd say.  Maybe for consistency, I should stick with R10 ...

Well, if you do that, I'll have to undo all my changes and push again.

 

But I don't know if anyone else has written code that assumes the register allocations. Anyone?

 

if it's just me, I'd say it's already done!

 

[+chue]

All of my unit tests pass with the 1.26 patches.

 

Here's just comparing the size for one of my test apps:

 

patch / size

1.25 "A" / 6528 bytes

1.25 "B" / 7552 bytes

1.26 / 6784 bytes

 

Much better in terms of size with the latest release.

 

No I don't assume anything in terms of register usage.  Normally I save any registers I use, and then restore them afterwards. I thought that the crt0 files might hard code the stack register, but in looking at a couple of mine (crt0.asm, crt0.c) they both use the alias "sp" instead of any register.

[PeteE]

13 hours ago, Tursi said:

But I don't know if anyone else has written code that assumes the register allocations. Anyone?

I did: Turmoil

 

The idea was to keep the address of the VDP address and write data port in registers R14 and R15:

// keep address and data addresses in global registers for fast access
register volatile u8 *vdp_address_reg_addr asm("r14");
register volatile u8 *vdp_write_data_reg_addr asm("r15");

...
  
	// initialize global register variables
	vdp_address_reg_addr = (u8*)0x8C02;
 	vdp_write_data_reg_addr = (u8*)0x8C00;

 

It's easy enough to change the register numbers for the new gcc patch, but I'm now getting warnings like this:

main.c:130: warning: call-clobbered register used for global register variable

I'll dig into this more later.

[JasonACT]

I've built the new GCC version, and I've successfully run the LibTI99 TESTLIB1/2/3 files produced by it - I didn't see anything bad happen, seemed to all work...

 

But I'm seeing an issue in @jedimatt42's Memtest with my 1MB SAMS - where it says writestring(4,29, int2str(j+7)); it writes a random number to the screen, if I change it to just (j) like the previous one, I can see the compiler removes the save temporary value to stack code (j+7, in fact +1..+7 are all used prior) and I get the expected number (j==0).

 

Then it crashes after it passes the first 3 blocks successfully and gets up to E000-FFFF.

 

Now, as this is the first time I've ever compiled TMS9900-GCC and done anything at all with it, it's probably something I'm doing wrong.

[Tursi]

1 hour ago, PeteE said:

I did: Turmoil

I'm perfectly willing to roll back the minor changes I made if you want to move the stack pointer back to R10... I only changed my CRT0 (to init both R10 and R15 as stack pointers) in libti99. I had to make changes to vgmcomp2 but for now I just wrapped the stack pointer register, so it's an easy rollback. The only other changes were in libti99all, and that's not at release state yet.

 

[JasonACT]

3 hours ago, JasonACT said:

Now, as this is the first time I've ever compiled TMS9900-GCC and done anything at all with it, it's probably something I'm doing wrong.

I compiled a version of GCC with the 1.19 patch and @jedimatt42's Memtest is working fine now.  I have no idea why the new GCC patch is doing this.

 

@Tursi I ran make clean on your library to rebuild a fresh version with the older GCC - I didn't notice it the first time around, but I think you're missing 4 object files in the makefile library list (4 "linefast" objects).

[JasonACT]

Native Windows (x32, so works on x64 too) TMS9900-GCC v1.19 attached.  Make sure the main bin directory is in your PATH.

 

install119.zip

[JasonACT]

Native Windows TMS9900-GCC v1.26 attached.

install126.zip

[Tursi]

2 hours ago, JasonACT said:

I compiled a version of GCC with the 1.19 patch and @jedimatt42's Memtest is working fine now.  I have no idea why the new GCC patch is doing this.

 

@Tursi I ran make clean on your library to rebuild a fresh version with the older GCC - I didn't notice it the first time around, but I think you're missing 4 object files in the makefile library list (4 "linefast" objects).

Hmm.. maybe. The main linefast will not work with the new compiler as it stands since it needs all the registers. Guess I can port the same fix as I did for the new libti99all.

 

I wonder, can I just check the compiler version in the code itself?

 

[Tursi]

So I looked into that, and I do have a couple of notes.

 

First is that since I was forced to check the older one, there is definitely a bug affecting the 64-column text code in libti99 - the printf tests fail pretty good. In fact there are several places that exhibit changed behaviour, and I did not notice this because I was only testing on the newer version, which has a heavier emphasis on char variables than the older one.

 

- the colors on the initial "Hello Bitmaps" screen are different

- the wipe color on the Pixel test screen is different

- however, the 64-column text code is correct

 

Tracking down all these bugs will take some effort. I also surprised myself that the drawlinefast code doesn't touch R15, so is accidentally safe. Ish. We'll see.

 

Second is that there appears to be only one define that contains the tms9900 patch level, and it's not being updated in these patches:

 

#define __VERSION__ "4.4.0 20090421 (TMS9900 patch 1.19)"

 

(This is from the new 1.26 patch - "-dM -E").

 

 

[+khanivore]

7 hours ago, JasonACT said:

I've built the new GCC version, and I've successfully run the LibTI99 TESTLIB1/2/3 files produced by it - I didn't see anything bad happen, seemed to all work...

 

But I'm seeing an issue in @jedimatt42's Memtest with my 1MB SAMS - where it says writestring(4,29, int2str(j+7)); it writes a random number to the screen, if I change it to just (j) like the previous one, I can see the compiler removes the save temporary value to stack code (j+7, in fact +1..+7 are all used prior) and I get the expected number (j==0).

 

Then it crashes after it passes the first 3 blocks successfully and gets up to E000-FFFF.

 

Now, as this is the first time I've ever compiled TMS9900-GCC and done anything at all with it, it's probably something I'm doing wrong.

Memtest crt0 sets up sp.  I just checked where sp is defined, and unfortunately, it is still defined as r10 in binutils-2.19.1/gas/config/tc-tms9900.c

 

(BTW I have the same results as you when I run the test.  I suspect because R15 is not iniitliased, it starts a at 0 and grows downward toward >F000 which then eventually collides the mem test and fun happens)

So your sp is not the right value which will very likely cause issues like you are seeing.  I think I'll just have to go back to using r10 as sp for consistency (sorry @Tursi)

Edited December 16, 2023 by khanivore
extra info

[+khanivore]

8 hours ago, PeteE said:

It's easy enough to change the register numbers for the new gcc patch, but I'm now getting warnings like this:

main.c:130: warning: call-clobbered register used for global register variable

I'll dig into this more later.

You need to tweak the FIXED_REGISTERS define in tms9900.h as well.  It's a bit fussy about how you define the registers.  Probably easiest if I just go back to using r10 as above

[+khanivore]

51 minutes ago, Tursi said:

 

Second is that there appears to be only one define that contains the tms9900 patch level, and it's not being updated in these patches:

 

#define __VERSION__ "4.4.0 20090421 (TMS9900 patch 1.19)"

 

(This is from the new 1.26 patch - "-dM -E").

 

 

Yeah I noticed that too.  I've added code yesterday to update that.  This is the WIP version of mkpatch.sh:
 

#!/bin/bash

VERSION=$1
if [ -z $VERSION ] ; then
   echo Error: Please specify a version number x.y[.z]
   exit
fi

echo `date +%Y%m%d` > dev/gcc-4.4.0/gcc/DATESTAMP
echo "TMS9900 patch $VERSION" > dev/gcc-4.4.0/gcc/DEV-PHASE

cd dev
diff -ru gcc-4.4.0-orig gcc-4.4.0 | grep -v "Only in gcc-4.4.0" > ../gcc-4.4.0-tms9900-$VERSION.patch
diff -ru binutils-2.19.1-orig binutils-2.19.1 | grep -v "Only in binutils-2.19.1" > ../binutils-2.19.1-tms9900-1.10.patch
cd ..

It now is the correct value when built:

 

4.4.0 20231214 (TMS9900 patch 1.27)

 

[Tursi]

I've finished my analysis for tonight...

 

I have a char->int promotion bug in the code that sets foreground and background color for bitmap mode, and this is a regression from the older compiler. I've detailed in the attached text file. But basically it's failing to promote an unsigned char to an int, then masking the two together, giving unreliable results.

 

The 64 column code needs more time. Although it appears to work even with the new compiler when the input parameter for character to print is an int, I can't find a bug in the char version and indeed the output data suggests that it probably isn't the character draw function at fault anyway. The same code also fails, in a different way, on the ColecoVision... so I am probably going to need to dig deeper and see what's being silly. That's not a super high priority for me at the moment.

 

The attached text file includes my 64 column disassembly because it does seem to be doing caller-saves on the registers (in at least once place), and I was expecting callee-saves. Not sure which way you expected it. But the only bug I'm reporting here is the color stuff at the top. 

wip3.txt

 

No worries if you change the registers back. I'll update my code when I pull it down and have it running.

 

[Tursi]

48 minutes ago, khanivore said:

Yeah I noticed that too.  I've added code yesterday to update that.  This is the WIP version of mkpatch.sh:

Awesome! Is there any possibility of making a second #define that contains just the TMS9900 patch level as an int, so it's a bit easier to compare mechanically?

 

I suppose it won't be too important with the stack register changing back, but in case of future breaking changes it'll make the code maintenance easier.

 

[+khanivore]

5 hours ago, Tursi said:

Awesome! Is there any possibility of making a second #define that contains just the TMS9900 patch level as an int, so it's a bit easier to compare mechanically?

 

I suppose it won't be too important with the stack register changing back, but in case of future breaking changes it'll make the code maintenance easier.

 

There isn't a standard way that I can see apart from extracting major and minor versions of GCC itself.  But I can see why you would want these as ints so I added these two custom defines:

__TMS9900_PATCH_MAJOR__=1
__TMS9900_PATCH_MINOR__=27

[+khanivore]

5 hours ago, Tursi said:

I've finished my analysis for tonight...

 

I have a char->int promotion bug in the code that sets foreground and background color for bitmap mode, and this is a regression from the older compiler. I've detailed in the attached text file. But basically it's failing to promote an unsigned char to an int, then masking the two together, giving unreliable results.

 

Execllent, thanks for that analysis, I have the issue reproduced.  It looks like the "andhi3" insn is misbehaving.  I hadn't touched that code but looking at it now it is over-writing operands which has caused problems in the past.  I'll refactor it and run some more tests.

[PeteE]

12 hours ago, khanivore said:

You need to tweak the FIXED_REGISTERS define in tms9900.h as well.  It's a bit fussy about how you define the registers.  Probably easiest if I just go back to using r10 as above

Actually, I think it relates to the CALL_USED_REGISTERS define.  With the current patch, only R14 doesn't produce the warning. With the old 1.18 patch, R9, R13-15 don't produce the warning.  If you do go back to SP in R10, I would suggest this for CALL_USED_REGISTERS:

#define CALL_USED_REGISTERS \
  {1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0}
/* SC 1  2  3  4  5  6  7  8  9  SP LR CB LW LP LS*/

That would allow me to use R14 and R15 for my global register variables.

 

EDIT: Nevermind the above, please go ahead and let GCC use all of the registers.  I discovered the -ffixed-REG option to gcc that allows you to update the fixed_regs table to avoid the warning.  So for global register variables in "r14" and "r15", I would add "-ffixed-r14 -ffixed-r15" to my CFLAGS.

 

Edited December 16, 2023 by PeteE
fixed-reg

[+khanivore]

Patch 1.27 has been release 

* Revert SP to R10 and BP to R9
* Add case to andhi3 to fix missed byte extend (bug manifested in libti99/vdp_bmcolor.c)
* Replace SRL with SWPB in trunc
* Added macros __TMS9900_PATCH_MAJOR__ and __TMS9900_PATCH_MINOR__ to query patch level

The issue with the bytewise and in bmcolor is that the optimiser (combine phase) tried to merge two insns, movqi and extend.  But it couldn't match extend with a memory reference (since I had extend constrained to registers only as it relies on sra) so it chucked it away and invoked andhi3 with a paradoxical subreg instead.  I still haven't found a way to tell it not to do that.  For now I'm detecting when andhi3 is passed a paradoxical subreg by seeing a non zero offset and emitting a swpb if it is.  Hopefully andhi3 is the only place this happens but can't guarantee it!

(I'm also not sure why it feels it has to upgrade 8-bit values to 16-bit values when doing binary AND.  It doesn't do that for OR ops)

tms9900-gcc-1.27.deb

[JasonACT]

Native Windows TMS9900-GCC v1.27 attached.

install127.zip

[+chue]

9 hours ago, khanivore said:

Patch 1.27 has been release

Looks good for me. All my unit tests passed