Thread overview | |||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Johannes Pfau Attachments:
| 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Iain Buclaw | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Johannes Pfau | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Johannes Pfau Attachments:
| 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 | ||||
---|---|---|---|---|
| ||||
Attachments:
| 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Iain Buclaw | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to David Nadlinger | 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 | ||||
---|---|---|---|---|
| ||||
Posted in reply to Johannes Pfau Attachments:
| 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 | ||||
---|---|---|---|---|
| ||||
Attachments:
| 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'; |
Copyright © 1999-2021 by the D Language Foundation