On 18 June 2015 at 23:07, Iain Buclaw <ibuc...@gdcproject.org> wrote:
> On 18 June 2015 at 22:18, Johannes Pfau via D.gnu <d.gnu@puremagic.com> > wrote: > >> Am Tue, 16 Jun 2015 22:05:44 +0200 >> schrieb Johannes Pfau <nos...@example.com>: >> >> > Am Thu, 11 Jun 2015 22:30:39 +0200 >> > schrieb "Iain Buclaw via D.gnu" <d.gnu@puremagic.com>: >> > >> > > On 11 June 2015 at 20:27, Johannes Pfau via D.gnu >> > > <d.gnu@puremagic.com> wrote: >> > > >> > > > Some recent change in GDC broke ARM cross compiler builds. Same >> > > > error with gcc-5.1 and gcc-4.9: >> > > > >> > > > https://gist.github.com/jpf91/1de81d6ff55587d702ae >> > > > >> > > > I'm not sure if this only happens for ARM cross compilers or for >> > > > all cross compilers. But it doesn't seem to happen with native >> > > > builds (or maybe it's target specific and it doesn't happen for >> > > > x86 builds). >> > > > >> > > > >> > > > Iain, any clue what could cause this? Otherwise I'll have to debug >> > > > this later on. >> > > > >> > > >> > > The comment above says: >> > > >> > > If the DECL isn't in memory, then the DECL wasn't properly >> > > marked TREE_ADDRESSABLE, which will be either a front-end >> > > or a tree optimizer bug. >> > > >> > > Peek returns a ubyte[4] - so my initial guess would be the recent >> > > NRVO change. Perhaps it wasn't quite as bullet-proof as I hoped. >> > > >> > >> > Small update: It's indeed the NRVO changes, reduced test case: >> > ------------------ >> > private union EndianSwapper >> > { >> > uint value; >> > ubyte[4] array; >> > } >> > >> > struct CRC32 >> > { >> > public: >> > @trusted pure nothrow finish() >> > { >> > auto tmp = peek(); >> > return tmp; >> > } >> > >> > @trusted pure nothrow peek() >> > { >> > EndianSwapper es; >> > es.value = 0; >> > return es.array; >> > } >> > } >> > ------------------ >> > >> >> @Iain please read my questions at the bottom of this message, the first >> (debugging) part probably isn't important. >> >> >> Debugging showed that the backend assigns a REG:SI rtx for tmp. >> However, tmp does have the ADDRESSABLE flag set: >> >> -------------------------------------------------------- >> <result_decl 0x7ffff7ff5078 tmp >> type <array_type 0x7ffff71f1738 >> type <integer_type 0x7ffff73e5f18 ubyte public unsigned QI >> size <integer_cst 0x7ffff73cdf00 constant 8> >> unit size <integer_cst 0x7ffff73cdf18 constant 1> >> align 8 symtab 0 alias set -1 canonical type 0x7ffff73e5f18 >> precision 8 min <integer_cst 0x7ffff73dd2e8 0> max <integer_cst >> 0x7ffff73dd2b8 255> pointer_to_this <pointer_type 0x7ffff71f17e0>> >> no-force-blk BLK size <integer_cst 0x7ffff73cddc8 constant 32> >> unit size <integer_cst 0x7ffff73cdde0 constant 4> >> align 8 symtab 0 alias set -1 canonical type 0x7ffff71f1738 >> domain <integer_type 0x7ffff71f1690 type <integer_type >> 0x7ffff73d10a8 sizetype> public SI size <integer_cst 0x7ffff73cddc8 >> 32> unit size <integer_cst 0x7ffff73cdde0 4> align 32 symtab 0 alias >> set -1 canonical type 0x7ffff71f1690 precision 32 min <integer_cst >> 0x7ffff73cddf8 0> max <integer_cst 0x7ffff71e34c8 3>> pointer_to_this >> <pointer_type 0x7ffff71fb2a0>> addressable used ignored regdecl BLK >> file >> /home/build/gdc-build-configs/configs/x86_64-linux-gnu/gcc-snapshot/arm-gdcproject-linux-gnueabi/.build/src/gcc-custom/libphobos/src/std/digest/crc.d >> line 10 col 14 size <integer_cst 0x7ffff73cddc8 32> unit size >> <integer_cst 0x7ffff73cdde0 4> align 8 context <function_decl >> 0x7ffff71f4000 finish> (reg:SI 110 [ <retval> ])> >> -------------------------------------------------------- >> >> >> The wrong RTL is generated in function.c:expand_function_start. >> expand_function_start checks aggregate_value_p (DECL_RESULT (subr), >> subr). aggregate_value_p does not look at TREE_ADDRESSABLE of the >> result decl, it only looks at TREE_ADDRESSABLE for the result type and >> the function type. (which could be a gcc bug?) >> >> Because of this we might have to set DECL_BY_REFERENCE for the return >> value as well. Will test this tomorrow. >> >> X86 might avoid this issue because of this: >> if (flag_pcc_struct_return && AGGREGATE_TYPE_P (type)) >> or this >> if (targetm.calls.return_in_memory (type, fntype)) >> >> >> >> However, writing this down and thinking about it now I've got one >> fundamental question: Can we even legally do NRVO in the posted test >> case? The return types are PODs and fit in a register. So from an API >> perspective they should be returned in registers unless otherwise >> requested by the user. We can probably set DECL_REFERENCE on the result >> to force this. But if we do this we change the ABI (register vs memory >> pointer) so we also need to be able to determine whether this function >> returns in memory when calling it. I don't see anything >> special about the function signature in this example which would allow >> this. >> >> Could it be possible the >> else if (TREE_CODE (return_type) == ARRAY_TYPE) ...BLKmode >> check isn't doing what it's supposed to do? >> Maybe we have to call if (targetm.calls.return_in_memory (type, fntype)) >> instead, like aggregate_value_p does. >> >> Also why is the extra array check necessary? Shouldn't >> aggregate_value_p handle arrays as well? >> > > For aggregate_value_p to work correctly, I think the function type should > be marked as addressable. > > I was just building gdc in a qemu raspbian chroot for other reasons, and > have just hit this. Will have a look. > > Iain. > > This is what I've settled with: https://github.com/D-Programming-GDC/GDC/pull/107 I've fixed the one failing testsuite test because GDC actually returns int[3] in registers (on 64bit at least). Iain
