#519 DS390 flat24: add struct/union return support

[Li-Hsin Chien]

This 4-patch series adds struct and union return support for the DS390 flat24 model.
This work is for B1a of the NGI0-funded SDCC project.

Scope:

• 10-bit hardware stack
• Direct calls and calls through function pointers
• Struct/union returns with --xstack and --stack-8bit are rejected for now

Tested:
make test-ds390

0 failures, 31971 tests, 6414 test cases.

Patches:

1. Refactor stack helper code
2. Add struct return support
3. Enable flat24 division functions
4. Run struct return regression tests

I would especially appreciate review of the struct return calling convention and the DPTR save/restore code in patch 2.

[Philipp Klaus Krause]

Patch 4:

• How about tests/dynamiccstructret.c.in?
• The comment in tests/bigreturn-remat.c.in is outdated, I think. AFAIK all ports (except unmaintained pic) now support struct parameters, so it is only missing struct return that stopped us from enabling this test.

Patch 3:

• I suggest to add a short comment at the new conditions, explaining them.

Patch 2:

• Looks like this should work, but I'm not familiar with ds390, maybe @maartenbrock could also have a quick look, since he known mcs51 well, and AFAIK ds390 architecture is somewhat similar to mcs51.
• If you want to make it a bit fancier (i.e. more efficient), I suggest to introduce a stackAdjust (int n, bool a_dead) function (like many other ports have) instead of emitDiscardStructReturnPointer, that you can also use for the parameter stack adjustment in genCall, and also do the struct return and stack parameter adjustement in one call to adjustStack.

Patch 1:

• Again I'm not familiar with ds390, but I wonder how that signed char offset in aopForSym works, when we have a 10 bit stack (and even for an 8-bit stack, wouldn't a signed char mean we can only access 127B on either side of the stack frame, instead of a total of 256B no matter on which side?) But that is not a regression, so not really an issue for this patch.

[Li-Hsin Chien]

Hi,
Thanks for the detailed review and suggestions!

Patch 4:
I enabled dynamiccstructret.c.in for ds390 and removed the old restriction in bigreturn-remat.c.in.

Patch 3:
I've added short comments explaining the new conditions for ds390 flat24.

Patch 2:
I updated the code based on Maarten’s feedback. The caller now passes the hidden result pointer in registers, not on the stack.

I added stackAdjust() to share the cleanup code in genCall() and genPcall(). The callee also uses it to remove its saved pointer. There is no longer a pointer on the caller’s stack to clean up with the arguments.
I kept the existing handling of A, so the helper does not need a_dead.

Patch 1:
I used Maarten’s refactoring for stack offsets and pointer handling, including the mcs51 changes.

[Maarten Brock]

Patch 1:

• For an 8-bit stack you can reach every location with both a signed and an unsigned char offset due to the wrap around nature of the stack pointer. And the char offset in aopForSym part is inside this if clause:

704:  if ((sym->onStack && !options.stack10bit) || sym->iaccess)
  705:   {

But I would make stackOffsetForSym() return short instead of int and remove a lot of casts.

[Maarten Brock]

And since mcs51 and ds390 are so very much alike, try to keep their code as close as possible. Thus this would be more appropriate:

/*-----------------------------------------------------------------*/
/* stackoffset - stack offset for symbol                           */
/*-----------------------------------------------------------------*/
static int
stackoffset (const symbol * sym)
{
  int offset = sym->stack;
  if (sym->stack < 0)
    offset -= _G.nRegsSaved;
  return offset;
}

[Maarten Brock]

Instead of your 0001-ds390-refactor-stack-helper-code.patch I propose this patch.
It has stackoffset() as mentioned above and modifies pointerCode() instead of introducing genericPointerSpaceTag().

It also beter optimizes aopForSym() stack offset calculations.

[Li-Hsin Chien]

Thank!
Thanks for your explanation of the 8-bit stack.
I used your proposed refactoring in v2 Patch 1, including stackoffset() and pointerCode(). The offsets in aopForSym() now use int without the old casts.

[Maarten Brock]

Patch 2:

• I would very much prefer that the result pointer is passed as the first parameter. I don't see why the result pointer should have to be on stack. As far as the parameter assignment goes

struct S foo(...)

then becomes

void foo(struct S *, ...)

IMHO this could work for all memory models and stack models.

[Li-Hsin Chien]

Thanks!
I changed Patch 2 so the caller passes the result pointer in dpl/dph/dpx/b, the registers used for the first parameter. The callee saves it until it returns.

[Li-Hsin Chien]

I've attached the four v2 patches.

[Philipp Klaus Krause]

Looks good to me (though I'm not a ds390 expert).
I wonder if adjustStack could be made slightly more efficient by doing "sp += -n" instead of "sp -= n", since addition is a bit cheaper than subtraction. But that would be an optimisation, and IMO the patches could also go in as-is.

[Li-Hsin Chien]

Thanks @spth !
I'll look into using addition instead of subtraction for the stack adjustment. (Will prepare a v3
Thanks for the suggestion and reivew!

@maartenbrock, could you also have a look at the v2 patches when you have time? I'd really appreciate your feedback!

Thanks again.