D.gnu - To POD or not to POD
- Johannes Pfau (23/23) Feb 12 2013 I've started debugging the unit test failures in std.datetime:
- Iain Buclaw (6/27) Feb 12 2013 TREE_ADDRESSABLE should be sufficient. I can't think any reason off the
- Johannes Pfau (16/19) Feb 13 2013 maybe TREE_ADDRESSABLE is too strong: It generates errors in the
- Iain Buclaw (9/28) Feb 13 2013 Don't set it on the variable, set it on the type.
- Johannes Pfau (11/44) Feb 13 2013 That's actually what I did. But the backend wants to create a copy of
- Iain Buclaw (20/64) Feb 13 2013 Ahh, that's because of this piece of codegen:
- Iain Buclaw (5/72) Feb 13 2013 After some more playing about, David's suggestion would be the quickest....
- Johannes Pfau (9/51) Feb 13 2013 Indeed. But what if the example is slightly adjusted to define a
- Iain Buclaw (7/54) Feb 13 2013 Backend doesn't know of those, and yes it would pass it in registers
- Iain Buclaw (7/54) Feb 13 2013 Backend doesn't know of those, and yes it would pass it in registers
- Iain Buclaw (6/35) Feb 13 2013 TREE_ADDRESSABLE (ctype) = !sym->isPOD() even :-)
- David Nadlinger (8/11) Feb 13 2013 I wouldn't spend too much time on implementing the old behavior -
- Johannes Pfau (10/24) Feb 13 2013 Good to know. This fixes the datetime specific instance of the problem.
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.
Feb 12 2013
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';
Feb 12 2013
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.
Feb 13 2013
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>: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';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.
Feb 13 2013
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: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 -----Am Tue, 12 Feb 2013 18:16:31 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:Don't set it on the variable, set it on the type. TypeStruct::toCtype() { TYPE_ADDRESSABLE(ctype) = !isPOD(); }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.
Feb 13 2013
Iain Buclaw <ibuclaw ubuntu.com> 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>: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';On 13 February 2013 13:26, Johannes Pfau <nospam example.com> wrote: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 -----Am Tue, 12 Feb 2013 18:16:31 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:Don't set it on the variable, set it on the type. TypeStruct::toCtype() { TYPE_ADDRESSABLE(ctype) = !isPOD(); }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.
Feb 13 2013
On 13 February 2013 15:20, Iain Buclaw <ibuclaw ubuntu.com> wrote:On 13 February 2013 14:35, Johannes Pfau <nospam example.com> wrote:After some more playing about, David's suggestion would be the quickest. ;-) -- Iain Buclaw *(p < e ? p++ : p) = (c & 0x0f) + '0';Am Wed, 13 Feb 2013 14:10:26 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>: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)>On 13 February 2013 13:26, Johannes Pfau <nospam example.com> wrote: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 -----Am Tue, 12 Feb 2013 18:16:31 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:Don't set it on the variable, set it on the type. TypeStruct::toCtype() { TYPE_ADDRESSABLE(ctype) = !isPOD(); }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.
Feb 13 2013
Am Wed, 13 Feb 2013 17:17:06 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:On 13 February 2013 15:20, Iain Buclaw <ibuclaw ubuntu.com> wrote:Indeed. But what if the example is slightly adjusted to define a destructor or postblit instead of the constructor? Does the backend know about those or would it still try to pass Date in registers? For this current test case the issue seems to be that DMD creates a StructLiteralExp for the default constructor call which is of course not an lvalue and gimple seems to dislike that. But I don't know how to fix this and there could be many more cases...After some more playing about, David's suggestion would be the quickest. ;-)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)>
Feb 13 2013
On Feb 13, 2013 6:55 PM, "Johannes Pfau" <nospam example.com> wrote:Am Wed, 13 Feb 2013 17:17:06 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:../../objdir-4.7/x86_64-unknown-linux-gnu/libphobos/dm-test.reduced/datetime2.d:22:On 13 February 2013 15:20, Iain Buclaw <ibuclaw ubuntu.com> wrote:Complete test case: https://gist.github.com/jpf91/4944999 -----Backend doesn't know of those, and yes it would pass it in registers because it will always see structs as a value type rather than a reference (though passing round 'this' should always be done in memory). Regards Iain.Indeed. But what if the example is slightly adjusted to define a destructor or postblit instead of the constructor? Does the backend know about those or would it still try to pass Date in registers?After some more playing about, David's suggestion would be the quickest. ;-)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)>
Feb 13 2013
On Feb 13, 2013 6:55 PM, "Johannes Pfau" <nospam example.com> wrote:Am Wed, 13 Feb 2013 17:17:06 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:../../objdir-4.7/x86_64-unknown-linux-gnu/libphobos/dm-test.reduced/datetime2.d:22:On 13 February 2013 15:20, Iain Buclaw <ibuclaw ubuntu.com> wrote:Complete test case: https://gist.github.com/jpf91/4944999 -----Backend doesn't know of those, and yes it would pass it in registers because it will always see structs as a value type rather than a reference (though passing round 'this' should always be done in memory). Regards Iain.Indeed. But what if the example is slightly adjusted to define a destructor or postblit instead of the constructor? Does the backend know about those or would it still try to pass Date in registers?After some more playing about, David's suggestion would be the quickest. ;-)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)>
Feb 13 2013
On 13 February 2013 14:10, Iain Buclaw <ibuclaw ubuntu.com> wrote:On 13 February 2013 13:26, Johannes Pfau <nospam example.com> wrote:TREE_ADDRESSABLE (ctype) = !sym->isPOD() even :-) Regards -- Iain Buclaw *(p < e ? p++ : p) = (c & 0x0f) + '0';Am Tue, 12 Feb 2013 18:16:31 +0000 schrieb Iain Buclaw <ibuclaw ubuntu.com>:Don't set it on the variable, set it on the type. TypeStruct::toCtype() { TYPE_ADDRESSABLE(ctype) = !isPOD();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.
Feb 13 2013
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
Feb 13 2013
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: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?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
Feb 13 2013