View mode: basic / threaded / horizontal-split · Log in · Help
February 12, 2013
To POD or not to POD
I've started debugging the unit test failures in std.datetime:

We have this Date struct:
-----
struct Date
{
   this(int a){}
   short _year  = 2;
   ubyte _month = 1;
   ubyte _day   = 1;
}
-----

It's passed to D runtime variadic functions. It's 4 bytes in total so
GCC passes this struct in registers on x86_64 and it's therefore saved
in reg_save_area.

But our va_arg implementation using TypeInfo calls TypeInfo.argTypes()
to check if the type can be passed in parameters. This check returns
false as it depends on the dmd check sym->isPOD. Therefore our va_arg
tries to load the Date instance from the overflow_arg / stack save area
instead of the register save area.

What would be the correct way to tell the gcc backend not to pass !isPOD
structs in registers? Using TREE_ADDRESSABLE?

OT: I think a simple constructor shouldn't prevent a type from being a
POD, but that should be defined by dmd /frontend.
February 12, 2013
Re: To POD or not to POD
On 12 February 2013 17:45, Johannes Pfau <nospam@example.com> wrote:

> I've started debugging the unit test failures in std.datetime:
>
> We have this Date struct:
> -----
> struct Date
> {
>     this(int a){}
>     short _year  = 2;
>     ubyte _month = 1;
>     ubyte _day   = 1;
> }
> -----
>
> It's passed to D runtime variadic functions. It's 4 bytes in total so
> GCC passes this struct in registers on x86_64 and it's therefore saved
> in reg_save_area.
>
> But our va_arg implementation using TypeInfo calls TypeInfo.argTypes()
> to check if the type can be passed in parameters. This check returns
> false as it depends on the dmd check sym->isPOD. Therefore our va_arg
> tries to load the Date instance from the overflow_arg / stack save area
> instead of the register save area.
>
> What would be the correct way to tell the gcc backend not to pass !isPOD
> structs in registers? Using TREE_ADDRESSABLE?
>
>
TREE_ADDRESSABLE should be sufficient.  I can't think any reason off the
top of my head why not.

-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
February 13, 2013
Re: To POD or not to POD
Am Tue, 12 Feb 2013 18:16:31 +0000
schrieb Iain Buclaw <ibuclaw@ubuntu.com>:

> TREE_ADDRESSABLE should be sufficient.  I can't think any reason off
> the top of my head why not.
> 

maybe TREE_ADDRESSABLE is too strong: It generates errors in the
backend if the frontend produces non-lvalues:
---
   auto b = Date();
   a(b);
---

works, but
---
   a(Date());
---

fails in gimplify.c. Do we really have to rewrite such cases so that
non-PODs get a temporary variable? And how would this be done? It seems
we would have to use the frontend for this, as maybeMakeTemp  and
makeTemp refuse to work for TREE_ADDRESSABLE types.
February 13, 2013
Re: To POD or not to POD
On Tuesday, 12 February 2013 at 17:45:11 UTC, Johannes Pfau wrote:
> OT: I think a simple constructor shouldn't prevent a type from 
> being a
> POD, but that should be defined by dmd /frontend.

I wouldn't spend too much time on implementing the old behavior - 
I think I managed to convince Walter that constructors preventing 
PODness is a bad idea in the last bigger discussion on the topic. 
He also mentioned what sounded like plans in this regard in a 
recent pull request.

Instead, I'd just comment out that specific check in isPOD.

David
February 13, 2013
Re: To POD or not to POD
On 13 February 2013 13:26, Johannes Pfau <nospam@example.com> wrote:

> Am Tue, 12 Feb 2013 18:16:31 +0000
> schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
>
> > TREE_ADDRESSABLE should be sufficient.  I can't think any reason off
> > the top of my head why not.
> >
>
> maybe TREE_ADDRESSABLE is too strong: It generates errors in the
> backend if the frontend produces non-lvalues:
> ---
>     auto b = Date();
>     a(b);
> ---
>
> works, but
> ---
>     a(Date());
> ---
>
> fails in gimplify.c. Do we really have to rewrite such cases so that
> non-PODs get a temporary variable? And how would this be done? It seems
> we would have to use the frontend for this, as maybeMakeTemp  and
> makeTemp refuse to work for TREE_ADDRESSABLE types.
>

Don't set it on the variable, set it on the type.

TypeStruct::toCtype()
{
   TYPE_ADDRESSABLE(ctype) = !isPOD();
}



-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
February 13, 2013
Re: To POD or not to POD
On 13 February 2013 14:10, Iain Buclaw <ibuclaw@ubuntu.com> wrote:

> On 13 February 2013 13:26, Johannes Pfau <nospam@example.com> wrote:
>
>> Am Tue, 12 Feb 2013 18:16:31 +0000
>> schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
>>
>> > TREE_ADDRESSABLE should be sufficient.  I can't think any reason off
>> > the top of my head why not.
>> >
>>
>> maybe TREE_ADDRESSABLE is too strong: It generates errors in the
>> backend if the frontend produces non-lvalues:
>> ---
>>     auto b = Date();
>>     a(b);
>> ---
>>
>> works, but
>> ---
>>     a(Date());
>> ---
>>
>> fails in gimplify.c. Do we really have to rewrite such cases so that
>> non-PODs get a temporary variable? And how would this be done? It seems
>> we would have to use the frontend for this, as maybeMakeTemp  and
>> makeTemp refuse to work for TREE_ADDRESSABLE types.
>>
>
> Don't set it on the variable, set it on the type.
>
> TypeStruct::toCtype()
> {
>     TYPE_ADDRESSABLE(ctype) = !isPOD();
>


TREE_ADDRESSABLE (ctype) = !sym->isPOD()   even :-)


Regards
-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
February 13, 2013
Re: To POD or not to POD
Am Wed, 13 Feb 2013 14:10:26 +0000
schrieb Iain Buclaw <ibuclaw@ubuntu.com>:

> On 13 February 2013 13:26, Johannes Pfau <nospam@example.com> wrote:
> 
> > Am Tue, 12 Feb 2013 18:16:31 +0000
> > schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
> >
> > > TREE_ADDRESSABLE should be sufficient.  I can't think any reason
> > > off the top of my head why not.
> > >
> >
> > maybe TREE_ADDRESSABLE is too strong: It generates errors in the
> > backend if the frontend produces non-lvalues:
> > ---
> >     auto b = Date();
> >     a(b);
> > ---
> >
> > works, but
> > ---
> >     a(Date());
> > ---
> >
> > fails in gimplify.c. Do we really have to rewrite such cases so that
> > non-PODs get a temporary variable? And how would this be done? It
> > seems we would have to use the frontend for this, as maybeMakeTemp
> > and makeTemp refuse to work for TREE_ADDRESSABLE types.
> >
> 
> Don't set it on the variable, set it on the type.
> 
> TypeStruct::toCtype()
> {
>     TYPE_ADDRESSABLE(ctype) = !isPOD();
> }

That's actually what I did. But the backend wants to create a copy of
the Date type which then fails as create_tmp_var fails for
TREE_ADDRESSABLE types.

Complete test case:
https://gist.github.com/jpf91/4944999

-----
../../objdir-4.7/x86_64-unknown-linux-gnu/libphobos/dm-test.reduced/datetime2.d:22:
internal compiler error: in create_tmp_var, at gimplify.c:479 0x804509
-----
February 13, 2013
Re: To POD or not to POD
Am Wed, 13 Feb 2013 15:14:36 +0100
schrieb "David Nadlinger" <see@klickverbot.at>:

> On Tuesday, 12 February 2013 at 17:45:11 UTC, Johannes Pfau wrote:
> > OT: I think a simple constructor shouldn't prevent a type from 
> > being a
> > POD, but that should be defined by dmd /frontend.
> 
> I wouldn't spend too much time on implementing the old behavior - 
> I think I managed to convince Walter that constructors preventing 
> PODness is a bad idea in the last bigger discussion on the topic. 
> He also mentioned what sounded like plans in this regard in a 
> recent pull request.
> 
> Instead, I'd just comment out that specific check in isPOD.
> 
> David

Good to know. This fixes the datetime specific instance of the problem.

But we should probably tell the gcc backend that non-POD types should
not be passed in registers anyway, right?

Even when ignoring constructors in the isPOD check, marking non-POD
types as TREE_ADDRESSABLE causes many errors in the test suite. Maybe
we also have to set TREE_ADDRESSABLE on function TYPE, CONSTRUCTOR,
VAR_DECL, PARM_DECL and RESULT_DECL nodes if a non-POD type is
involved, but I'd expect the backend to do that automatically?
February 13, 2013
Re: To POD or not to POD
On 13 February 2013 14:35, Johannes Pfau <nospam@example.com> wrote:

> Am Wed, 13 Feb 2013 14:10:26 +0000
> schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
>
> > On 13 February 2013 13:26, Johannes Pfau <nospam@example.com> wrote:
> >
> > > Am Tue, 12 Feb 2013 18:16:31 +0000
> > > schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
> > >
> > > > TREE_ADDRESSABLE should be sufficient.  I can't think any reason
> > > > off the top of my head why not.
> > > >
> > >
> > > maybe TREE_ADDRESSABLE is too strong: It generates errors in the
> > > backend if the frontend produces non-lvalues:
> > > ---
> > >     auto b = Date();
> > >     a(b);
> > > ---
> > >
> > > works, but
> > > ---
> > >     a(Date());
> > > ---
> > >
> > > fails in gimplify.c. Do we really have to rewrite such cases so that
> > > non-PODs get a temporary variable? And how would this be done? It
> > > seems we would have to use the frontend for this, as maybeMakeTemp
> > > and makeTemp refuse to work for TREE_ADDRESSABLE types.
> > >
> >
> > Don't set it on the variable, set it on the type.
> >
> > TypeStruct::toCtype()
> > {
> >     TYPE_ADDRESSABLE(ctype) = !isPOD();
> > }
>
> That's actually what I did. But the backend wants to create a copy of
> the Date type which then fails as create_tmp_var fails for
> TREE_ADDRESSABLE types.
>
> Complete test case:
> https://gist.github.com/jpf91/4944999
>
> -----
>
> ../../objdir-4.7/x86_64-unknown-linux-gnu/libphobos/dm-test.reduced/datetime2.d:22:
> internal compiler error: in create_tmp_var, at gimplify.c:479 0x804509
> -----
>
>

Ahh, that's because of this piece of codegen:

SAVE_EXPR <*test.Date.this (&((void) (__ctmp971 = _D4test4Date6__initZ);,
__ctmp971), 0)>


Gimple doesn't like the dereference in SAVE_EXPR.  This should work.

tree
IRState::makeTemp (tree t)
{
 tree type = TREE_TYPE (t);
 if (TREE_CODE (type) != ARRAY_TYPE && !TREE_ADDRESSABLE (type))
   return save_expr (t);

 return stabilize_reference (t);
}


So the generated code is now:

*SAVE_EXPR <test.Date.this (&((void) (__ctmp971 = _D4test4Date6__initZ);,
__ctmp971), 0)>



Regards
-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
February 13, 2013
Re: To POD or not to POD
On 13 February 2013 15:20, Iain Buclaw <ibuclaw@ubuntu.com> wrote:

> On 13 February 2013 14:35, Johannes Pfau <nospam@example.com> wrote:
>
>> Am Wed, 13 Feb 2013 14:10:26 +0000
>> schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
>>
>> > On 13 February 2013 13:26, Johannes Pfau <nospam@example.com> wrote:
>> >
>> > > Am Tue, 12 Feb 2013 18:16:31 +0000
>> > > schrieb Iain Buclaw <ibuclaw@ubuntu.com>:
>> > >
>> > > > TREE_ADDRESSABLE should be sufficient.  I can't think any reason
>> > > > off the top of my head why not.
>> > > >
>> > >
>> > > maybe TREE_ADDRESSABLE is too strong: It generates errors in the
>> > > backend if the frontend produces non-lvalues:
>> > > ---
>> > >     auto b = Date();
>> > >     a(b);
>> > > ---
>> > >
>> > > works, but
>> > > ---
>> > >     a(Date());
>> > > ---
>> > >
>> > > fails in gimplify.c. Do we really have to rewrite such cases so that
>> > > non-PODs get a temporary variable? And how would this be done? It
>> > > seems we would have to use the frontend for this, as maybeMakeTemp
>> > > and makeTemp refuse to work for TREE_ADDRESSABLE types.
>> > >
>> >
>> > Don't set it on the variable, set it on the type.
>> >
>> > TypeStruct::toCtype()
>> > {
>> >     TYPE_ADDRESSABLE(ctype) = !isPOD();
>> > }
>>
>> That's actually what I did. But the backend wants to create a copy of
>> the Date type which then fails as create_tmp_var fails for
>> TREE_ADDRESSABLE types.
>>
>> Complete test case:
>> https://gist.github.com/jpf91/4944999
>>
>> -----
>>
>> ../../objdir-4.7/x86_64-unknown-linux-gnu/libphobos/dm-test.reduced/datetime2.d:22:
>> internal compiler error: in create_tmp_var, at gimplify.c:479 0x804509
>> -----
>>
>>
>
> Ahh, that's because of this piece of codegen:
>
> SAVE_EXPR <*test.Date.this (&((void) (__ctmp971 = _D4test4Date6__initZ);,
> __ctmp971), 0)>
>
>
> Gimple doesn't like the dereference in SAVE_EXPR.  This should work.
>
> tree
> IRState::makeTemp (tree t)
> {
>   tree type = TREE_TYPE (t);
>   if (TREE_CODE (type) != ARRAY_TYPE && !TREE_ADDRESSABLE (type))
>     return save_expr (t);
>
>   return stabilize_reference (t);
> }
>
>
> So the generated code is now:
>
> *SAVE_EXPR <test.Date.this (&((void) (__ctmp971 = _D4test4Date6__initZ);,
> __ctmp971), 0)>
>
>
After some more playing about, David's suggestion would be the quickest. ;-)

-- 
Iain Buclaw

*(p < e ? p++ : p) = (c & 0x0f) + '0';
« First   ‹ Prev
1 2
Top | Discussion index | About this forum | D home