flexspin compiler for P2: Assembly, Spin, BASIC, and C in one compiler

ersmith

@Wuerfel_21 said:
Does that mean there's a problem with signed char in bytecode C, too? That should really work. Though idk, that'd involve messing with, BCCompileMemOpExEx which is the sort of code that should probably be completely trashed and rewritten.

I think signed char usually ends up working because the integer type system (usually) inserts the casts it needs, and if it doesn't then it (usually) doesn't matter because the lower 8 bits come out correctly anyway. There might be a few edge cases where it fails though, probably in comparisons.

signed boolean is an odd beast though because it's really just a 1 bit value, so we can't rely on the lower 8 bits being correct and have to sign extend it on every load/store.

evanh

I'm seeing an opportunity for a small but recurring optimisation. There is lots of the following type sequence in the compiled code:

    add ptr__dat__, #12
    mov arg01, ptr__dat__
    sub ptr__dat__, #12

It seems to me the following would do the job better:

    mov arg01, ptr__dat__
    add arg01, #12

iseries

@evanh ,
I don't know, but to me that code looks right. arg01 is variable in your function and you're passing an array to it.

In this case the arg01 is a pointer to the array and not an element in the array so instead of a Load it's doing a Mov.

Changing that stack behavior could lead to other optimization issues.

Mike

ersmith

@evanh : Thanks for the tip. In general the longer sequence is necessary because at a low level the compiler doesn't quite know what its going to do later with ptr__dat__, but in many cases it looks like we could probably do a peephole optimization on the final output to convert it to the shorter form you showed.

evanh

Thanks Eric.

Mike,
Apologies, I just cut and pasted that one. The compiler's choice of working registers sometimes seems odd too. I could have used a better example using var01 instead of arg01 to more clearly demonstrate what I was pointing out.

Wuerfel_21

@evanh said:
The compiler's choice of working registers sometimes seems odd too.

That's because of an optimization I added (flag is -Olocal-reuse). It basically tries to re-assign the usual work registers to ones that are lying around unused (argxx being a prime candidate). This often reduces the amount of registers that need to be saved to the stack (and the amount of cogRAM allocated overall)

evanh

And another one. Unneeded doubling up on zero extending when the rdbyte has already zero extended the value in arg01.

    rdbyte  arg01, arg01
    getbyte arg01, arg01, #0

This is reading an uint8_t byte from hubRAM and directly used as an uint32_t pinfield in a _pinstart() function:

    _pinstart( drv->pinclk, P_PULSE | P_OE | P_INVERT_OUTPUT | P_SCHMITT_A,
               rc | rc/2<<16, 0 );    // SD clock gen smartpin

PS: Flexspin Version 6.9.0-beta-v6.8.1-23-g501f4855 Compiled on: Mar 15 2024
Probably should update it ...

evanh

Compiler lock-up bug. It don't finish compiling with -O1 or -O2. Compiles with -O0.

void  main( void )
{
    int  ticks;

    ticks = _cnt();
    while( _pinr(56) );
    ticks = _cnt() - ticks;

    printf("CMD pin level = %d\n", _pinr(56));
    ticks = _muldiv64(ticks, 1000000, _clockfreq());
}

Wuerfel_21

@evanh said:
Compiler lock-up bug. It don't finish compiling with -O1 or -O2. Compiles with -O0.

You can narrow it down further by disabling individual features - try -O1,~local-reuse or -O1,~cordic-reorder (those two are the candidates for infinite looping passes that come to mind, anyways)

evanh

@Wuerfel_21 said:

@evanh said:
Compiler lock-up bug. It don't finish compiling with -O1 or -O2. Compiles with -O0.

You can narrow it down further by disabling individual features - try -O1,~local-reuse or -O1,~cordic-reorder (those two are the candidates for infinite looping passes that come to mind, anyways)

Cool, compiles and runs with -O1,~cordic-reorder. Which makes sense because the bug happened the moment I added the _muldiv64();

ersmith

@evanh said:

@Wuerfel_21 said:

@evanh said:
Compiler lock-up bug. It don't finish compiling with -O1 or -O2. Compiles with -O0.

You can narrow it down further by disabling individual features - try -O1,~local-reuse or -O1,~cordic-reorder (those two are the candidates for infinite looping passes that come to mind, anyways)

Cool, compiles and runs with -O1,~cordic-reorder. Which makes sense because the bug happened the moment I added the _muldiv64();

Two optimizations where undoing each other (the CORDIC wanted to put a wrlong after a qmul, but that put it next to a rdlong, so another optimization moved it back before the qmul). Fixed now in github.

evanh

Working, thank you Eric.

Wuerfel_21

@ersmith said:
Two optimizations where undoing each other (the CORDIC wanted to put a wrlong after a qmul, but that put it next to a rdlong, so another optimization moved it back before the qmul). Fixed now in github.

That other pass is pretty much pointless on P2 to begin with (and on P1 cogexec (LMM doesn't care) you really want 2+4n other ops inbeteween RDxxxx/WRxxxx, not just 1 (but it's better than 0)).

TonyB_

@Wuerfel_21 said:

@ersmith said:
Two optimizations where undoing each other (the CORDIC wanted to put a wrlong after a qmul, but that put it next to a rdlong, so another optimization moved it back before the qmul). Fixed now in github.

That other pass is pretty much pointless on P2 to begin with (and on P1 cogexec (LMM doesn't care) you really want 2+4n other ops inbetween RDxxxx/WRxxxx, not just 1 (but it's better than 0)).

If hub RAM slice is the same, three instructions between RDxxxx and WRxxxx take no more cycles than two, one or none.

__deets__

I'm trying to convert a rp2040 CAN bus implementation to the P2, see https://github.com/kentindell/canis-can-sdk/blob/main/canapi.h

After some initial success I end up with this code

enum { _clkfreq = 200_000_000, _xtlfreq = 20_000_000 };


#include <sys/size_t.h>

#include <stdio.h>
#include <propeller2.h>

typedef struct {
    uint32_t id;
} can_id_t;

can_id_t can_make_id(bool extended, uint32_t arbitration_id)
{
  return can_id_t{0};
}

void main() {
  _setbaud(2_000_000);

}

giving me this error:

avionic.cpp:29: error: syntax error, unexpected type name `can_id_t'

I'm confused as to what's going on there. Is returning structs in general not supported?

iseries

@deets ,

Whoa even Visual Studio complains about that one.
Use:

can_id_t whatever;
  whatever.id = 0;
return whatever;

That always gets me. If a structure is on the stack and you return it why doesn't the stack eat it.

Mike

ersmith

@deets said:
I'm trying to convert a rp2040 CAN bus implementation to the P2, see https://github.com/kentindell/canis-can-sdk/blob/main/canapi.h

After some initial success I end up with this code

...

giving me this error:

avionic.cpp:29: error: syntax error, unexpected type name `can_id_t'

I'm confused as to what's going on there. Is returning structs in general not supported?

Returning structs is certainly supported, but not all of C++ is supported.

The line in question is:

   return can_id_t{0};

which is not valid C code (it probably is valid C++ code, but flexspin only implements a small subset of C++). gcc also complains about that line, although g++ is OK with it.

If you re-write it as:

    return (can_id_t){0};

then it will work.

__deets__

Thanks, that helped me move further. C++ is in fact my main driver, so I guess I'm not really aware of quite a few of these subtle pitfalls. Maybe this one is similar:

I'm now struggling with a global variable that's used as null-initializer.

This is the original declaration:

https://github.com/kentindell/canis-can-sdk/blob/7a5a520abc8bef4ff2a6f68e38de1a67e623709e/canapi.h#L98

it's used e.g. here:

https://github.com/kentindell/canis-can-sdk/blob/7a5a520abc8bef4ff2a6f68e38de1a67e623709e/mcp25xxfd/mcp25xxfd.c#L751

When I use it as-is, I get the error

superprop -g build
/opt/flexspin/bin/flexcc -2 -Wall -g -o avionic.binary avionic.cpp

../canapi.h:82: error: Redefining symbol can_uref_null
canapi.h:83: error: Previous definition here

I see in the stdlib.h for example that there is some usage of the extern keyword, e.g. in time.h, but I'm not really enlightened what to do here.

EDIT

So looking a bit more into this I see that canapi.h is included several times, and it appears as if the include guard it clearly has is somehow now working?

In my avionics.cpp, I include canapi.h:

#include "canapi.h"

This declares a function inside mcp25xxfd.c

can_errorcode_t can_setup_controller(can_controller_t *controller,
                                     const can_bitrate_t *bitrate,
                                     const can_id_filters_t *all_filters,
                                     can_mode_t mode,
                                     uint16_t options)  __fromfile("mcp25xxfd/mcp25xxfd.c");

The .c includes canapi.h again:

// Brings in API, chip-specific and board-specific definitions
#include "../canapi.h"

When I add this to this include

#define FOOBARBAZ

// Brings in API, chip-specific and board-specific definitions
#include "../canapi.h"

I would've expected this essentially to be a no-op, because the canapi include guard should've kicked in, no? But it isn't, I've added

#ifdef FOOBARBAZ
#error "double include?"
#endif

to canapi.h, and the error is triggered.

__deets__

Ok, I looked a bit deeper, and this circular dependency is somehow a problem. I also get warnings like

incompatible pointer types in parameter passing: expected pointer to _struct___anon_f63c68e300000023 but got pointer to _struct___anon_309768ae00000023

where the same typedef is re-evaluated it appears.

Is there an example of a bigger code base for flexcc I can study? I've read the .md-files a couple of times but haven't found anything.
Or is this __fromfile something reserved for the standard library somehow, and I need to run my compiler invocation differently?

ersmith

You probably shouldn't be using __fromfile at all, unless you have a very specific need. Normally you'd either use the command line version of flexcc and put all the files on the command line, use a Makefile, or else create a project file (.fpide file) that includes all the files that you want to compile. __fromfile is non-standard and is intended for implementing libraries.

evanh

Feature request ...
I've found I can use the LOC instruction, in inline pasm, for embedding an immediate hubRAM reference to variables/arrays/structs, which is nice. But it can't handle me also embedding an offset to that reference. I would love to say I want the address of the second byte of the third longword in array int32_t resp[10];. eg: loc pb,#resp[2]+1. Or, maybe simpler like this: loc pb,#resp+9

ManAtWork

While trying to port the EtherCAT slave stack code to the P2 I've stumbled across this... Flexspin seems to not like if all functions in a .c file are completely empty. It exits with error code 1 after compiling P2_HAL.c. As a first step I'm about to remove all calls to PIC32 system libraries until everything compiles without errors. Then I plan to implement all hardware dependant things like interrupts for the P2, again. Please ignore the ugly code, I know it is full of errors. It would just be good if the compiler gave a message where it found a problem instead of just exiting.

evanh

Looks like similar issue to what Deets hit above. There is a __fromfile ("P2_HAL.c"); in the P2_HAL.h header.

__deets__

@ersmith said:
You probably shouldn't be using __fromfile at all, unless you have a very specific need. Normally you'd either use the command line version of flexcc and put all the files on the command line, use a Makefile, or else create a project file (.fpide file) that includes all the files that you want to compile. __fromfile is non-standard and is intended for implementing libraries.

That did the trick, I have working code now. Thanks. I do get these warnings:

avionic.cpp:29: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:29: warning: incompatible pointer types in parameter passing: expected pointer to const _struct___anon_bb4c68ec000000c9 but got pointer to _struct___anon_7b6e68f2000000c9
avionic.cpp:29: warning: incompatible pointer types in parameter passing: expected pointer to const _struct___anon_bb4c68ec00000126 but got pointer to _struct___anon_7b6e68f200000126
avionic.cpp:49: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:52: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:52: warning: incompatible pointer types in parameter passing: expected pointer to const _struct___anon_bb4c68ec00000101 but got pointer to _struct___anon_7b6e68f200000101
avionic.cpp:71: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:71: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec00000111 but got pointer to _struct___anon_7b6e68f200000111

I can't observe any immediate negative side-effects so far, but wonder if this is somehow fixable.

The code is difficult to share (at least half a dozen files), but if needed I can of course do that.

ersmith

@evanh said:
Feature request ...
I've found I can use the LOC instruction, in inline pasm, for embedding an immediate hubRAM reference to variables/arrays/structs, which is nice. But it can't handle me also embedding an offset to that reference. I would love to say I want the address of the second byte of the third longword in array int32_t resp[10];. eg: loc pb,#resp[2]+1. Or, maybe simpler like this: loc pb,#resp+9

loc pb,#resp+9 is supported now

@ManAtWork said:
While trying to port the EtherCAT slave stack code to the P2 I've stumbled across this... Flexspin seems to not like if all functions in a .c file are completely empty. It exits with error code 1 after compiling P2_HAL.c. As a first step I'm about to remove all calls to PIC32 system libraries until everything compiles without errors. Then I plan to implement all hardware dependant things like interrupts for the P2, again. Please ignore the ugly code, I know it is full of errors. It would just be good if the compiler gave a message where it found a problem instead of just exiting.

It was crashing rather than exiting . That crash should be fixed now in 6.9.7, which is on github. Thanks for the bug report.

evanh

Way to go! Thanks Eric.

ersmith

@deets said:

@ersmith said:
You probably shouldn't be using __fromfile at all, unless you have a very specific need. Normally you'd either use the command line version of flexcc and put all the files on the command line, use a Makefile, or else create a project file (.fpide file) that includes all the files that you want to compile. __fromfile is non-standard and is intended for implementing libraries.

That did the trick, I have working code now. Thanks. I do get these warnings:

avionic.cpp:29: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:29: warning: incompatible pointer types in parameter passing: expected pointer to const _struct___anon_bb4c68ec000000c9 but got pointer to _struct___anon_7b6e68f2000000c9
avionic.cpp:29: warning: incompatible pointer types in parameter passing: expected pointer to const _struct___anon_bb4c68ec00000126 but got pointer to _struct___anon_7b6e68f200000126
avionic.cpp:49: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:52: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:52: warning: incompatible pointer types in parameter passing: expected pointer to const _struct___anon_bb4c68ec00000101 but got pointer to _struct___anon_7b6e68f200000101
avionic.cpp:71: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec0000012c but got pointer to _struct___anon_7b6e68f20000012c
avionic.cpp:71: warning: incompatible pointer types in parameter passing: expected pointer to _struct___anon_bb4c68ec00000111 but got pointer to _struct___anon_7b6e68f200000111

I can't observe any immediate negative side-effects so far, but wonder if this is somehow fixable.

Probably using some typedefs or perhaps even putting names on structs (so struct foo { ... instead of struct {) will fix it.

The code is difficult to share (at least half a dozen files), but if needed I can of course do that.

If it's just a matter of figuring out which files to share, flexspin has a --zip file to create a zip of the project. Just compile as you normally would but add a --zip to the command line, and it should produce a .zip file with everything in it.

evanh

Oh, darn, it's broken. The dat offset is wholesale replaced instead of being added to.

The old code still works:

static uint32_t  respval( void )
{
    uint32_t  rc;

    __asm {
        loc pb, #resp    // 
        add pb, #1
        rdlong  rc, pb
        movbyts rc, #0b00_01_10_11
    }
    return rc;
}

It produces this inline pasm:

    ...
    mov local01, result1 wz
 if_ne  loc pb, #@_dat_ + 20
 if_ne  add pb, #1
 if_ne  rdlong  local01, pb
 if_ne  movbyts local01, #27

Changing to the newer feature:

static uint32_t  respval( void )
{
    uint32_t  rc;

    __asm {
        loc pb, #resp+1
        rdlong  rc, pb
        movbyts rc, #0b00_01_10_11
    }
    return rc;
}

Produces this inline pasm:

    ...
    mov local01, result1 wz
 if_ne  loc pb, #@_dat_ + 1
 if_ne  rdlong  local01, pb
 if_ne  movbyts local01, #27

ersmith

@evanh said:
Oh, darn, it's broken. The dat offset is wholesale replaced instead of being added to.

Aargh! It should be fixed now in the github sources. Thanks for catching this.

evanh

Thanks again. Funnily, now that I've made it into a function, which it hadn't been prior to posting here, it's become apparent what I've got is a straight forward big-endian byte-swap loading function. The address offset is important too but that kind of goes with the territory of the function interface. End result is I probably won't use the new LOC feature here. It would still save an instruction on each use though.

static uint32_t  loadbe( uint8_t *const buf )
{
    uint32_t  val;

    __asm {
        rdlong  val, buf
        movbyts val, #0b00_01_10_11
    }
    return val;
}