#4153 mcs51: positionRegs() swaps a live operand to match a spilt result

[d00f]

With 4.5.0, 4.6.0 and trunk r17011, -mmcs51 can compute the address of a 16-bit array element with its high byte taken from the low byte of the result. The attached test shows it in mcs51-small and mcs51-large: id[3] stays 0 because the store lands somewhere else in xdata.

void update(uint8_t port) __reentrant
{
  if (timer[port] && check(port))
    if (timer[port])
      flags &= 0;
  memcpy(ent_a[port].mac, pkt, 6);
  ent_b[port].prio = id[port] = ((struct pkt *)pkt)->id;
  cost[port] = scratch;
  timer[port] = port;
}

With sdcc -mmcs51 -S on the test (plus -I for testfwk.h) the code for id[port] is:

mov a,r3
add a,#_id
mov r4,a
addc a,#(_id >> 8)
mov r3,a

The offset is in r3:r4 and the result in r4:r3. Tracing positionRegs() shows the reverse pass swapping the offset to match a result that spilSomething() had spilt. That result still holds its old regs[], so the swap undoes the earlier match with the real result.

The patch adds the result->isspilt check that ds390 got in r10200. With it the new test no longer fails, and the mcs51-small and mcs51-small-stack-auto regression runs show no new failures.

[d00f]

The patch against r17011 is attached. It has the one-line change in src/mcs51/ralloc.c and the test as support/regression/tests/bug-4153.c. Without the change the test fails on id[3] in mcs51-small and mcs51-large, while mcs51-small-stack-auto does not trigger it.

[d00f]

A short follow-up in case it makes this easier to look at. Below is a standalone version of the test, without the regression framework. It prints PASS or FAIL through the ucsim simif interface:

sdcc -mmcs51 repro-4153.c
printf 'run\nquit\n' | s51 -t 8052 -I 'if=xram[0x7654]' repro-4153.ihx

With plain 4.5.0 it prints FAIL in the small and large models, and with the patch from this ticket it prints PASS. I also tried shrinking it with cvise, but the function under test barely got smaller, so it seems to need all of those statements to run out of registers.

#include <stdint.h>
#include <string.h>

struct entry {
  uint8_t prio;
  uint8_t mac[6];
};

struct pkt {
  uint8_t id;
};

__xdata struct entry ent_a[4], ent_b[4];
__xdata uint16_t id[4];
__xdata uint32_t cost[4];
__xdata uint16_t timer[4];
__xdata uint32_t scratch;
uint16_t flags;
uint8_t *pkt;
uint8_t buf[8];

uint8_t check(uint8_t port) __reentrant;

void update(uint8_t port) __reentrant
{
  if (timer[port] && check(port))
    if (timer[port])
      flags &= 0;
  memcpy(ent_a[port].mac, pkt, 6);
  ent_b[port].prio = id[port] = ((struct pkt *)pkt)->id;
  cost[port] = scratch;
  timer[port] = port;
}

uint8_t check(uint8_t port) __reentrant
{
  return port == 0xff;
}

#define SIMIF (*(volatile __xdata char *)0x7654)

static void say(const char *s)
{
  while (*s) {
    SIMIF = 'p';
    SIMIF = *s++;
  }
}

void main(void)
{
  uint8_t i;

  for (i = 0; i < 8; i++)
    buf[i] = i + 0x10;
  pkt = buf;
  scratch = 0x12345678;
  timer[3] = 1;
  update(3);
  say(id[3] == 0x10 ? "PASS\n" : "FAIL: id[3] not stored\n");
  SIMIF = 's';
  for (;;)
    ;
}

If another form of report or patch would be easier for you to review, just let me know and I'll redo it. Thanks for taking a look.

[Philipp Klaus Krause]

I can reproduce the issue using SDCC built from current trunk on my Debian GNU/Linux testing system.
Clearly an issue in the interaction of register allocation and codegen, as wen can see in the asm (compiled with --no-peep --i-code-in-asm), since the register allocator places variables in register in a way that codegen can't handle:

;   [---34--7] ic:47:   iTemp29 [k46 lr29:33 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-int __xdata* fixed}{ sir@ _update_sloc4_1_0}[r4 r3 ] = iTemp27 [k44 lr28:29 so:0]{ ia0 a2p0 re0 rm1 nos0 ru0 dp0 oe0 myp0}{unsigned-int __xdata* fixed}[remat] + iTemp2 [k6 lr5:29 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0 oe0 myp0}{unsigned-int fixed}{ sir@ _update_sloc4_1_0}[r3 r4 ]
;   genPlus
    mov a,r3
    add a, #_id
    mov r4,a
    mov a,r4
    addc    a, #(_id >> 8)
    mov r3,a

[Maarten Brock]

Patch is applied in [r17025].

[d00f]

The patch from [bugs:#4154] is independent of this one and still applies cleanly on top of [r17025], in case it helps there too.

Thanks for applying this one, and to Philipp for confirming it on trunk.

P.S.: Edited by @spth to fix the links.