New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use literal instead of symbol for init #2146

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@WalterBright
Member

WalterBright commented Jun 7, 2013

which enables better code to be generated for small wrapper structs, i.e.:

mov EAX,3

instead of:

mov EAX,S.init

for:

struct S { int a = 3; this(int I) { } }
S s = S(1);
@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 7, 2013

Member

This fails CTFE on this example:

struct Bug7940 {
    int m;
}

struct App7940 {
   Bug7940[] x;
}

int bug7940() {
  Bug7940[2] y;
  App7940 app;
  app.x = y[0..1];
  app.x[0].m = 12;
  assert(y[0].m == 12);
  assert(app.x[0].m == 12);
  return 1;
}

static assert(bug7940());   // fails CTFE

void main()
{
    assert(bug7940());  //  works at runtime
}
Member

WalterBright commented Jun 7, 2013

This fails CTFE on this example:

struct Bug7940 {
    int m;
}

struct App7940 {
   Bug7940[] x;
}

int bug7940() {
  Bug7940[2] y;
  App7940 app;
  app.x = y[0..1];
  app.x[0].m = 12;
  assert(y[0].m == 12);
  assert(app.x[0].m == 12);
  return 1;
}

static assert(bug7940());   // fails CTFE

void main()
{
    assert(bug7940());  //  works at runtime
}
@donc

View changes

Show outdated Hide outdated src/declaration.c Outdated
@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Jun 8, 2013

Member

This feels wrong to me, shouldn't the backend be constant-folding this? If the data is known to never change, why can't the optimizer generate the fast code by itself? Then other code can benefit from the optimization. (eg load from variable) If the initialization data is not available to the backend, then maybe the way data is presented is the problem. I can't see how this belongs anywhere further forward than the glue layer.

Member

yebblies commented Jun 8, 2013

This feels wrong to me, shouldn't the backend be constant-folding this? If the data is known to never change, why can't the optimizer generate the fast code by itself? Then other code can benefit from the optimization. (eg load from variable) If the initialization data is not available to the backend, then maybe the way data is presented is the problem. I can't see how this belongs anywhere further forward than the glue layer.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 8, 2013

Member

Initialization with a global variable rather than a literal is something the optimizer can't look inside.

Member

WalterBright commented Jun 8, 2013

Initialization with a global variable rather than a literal is something the optimizer can't look inside.

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Jun 8, 2013

Member

Can't or doesn't? If the global is known to never change, like in this case, what is the problem?

Member

yebblies commented Jun 8, 2013

Can't or doesn't? If the global is known to never change, like in this case, what is the problem?

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 8, 2013

Member

Typing it is 'const' makes everything it refers to const, too.

Member

WalterBright commented Jun 8, 2013

Typing it is 'const' makes everything it refers to const, too.

@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Jun 8, 2013

Member

Typing it is 'const' makes everything it refers to const, too.

So don't type it as const - just tell the backend that it never changes. This is internal to the compiler, so it doesn't need to work within D's type system.

Member

yebblies commented Jun 8, 2013

Typing it is 'const' makes everything it refers to const, too.

So don't type it as const - just tell the backend that it never changes. This is internal to the compiler, so it doesn't need to work within D's type system.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 8, 2013

Member

I'd rather do it this way, and besides, this way will work with all the backends.

Member

WalterBright commented Jun 8, 2013

I'd rather do it this way, and besides, this way will work with all the backends.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 8, 2013

Member

This bug needs fixing before this pull can work:

http://d.puremagic.com/issues/show_bug.cgi?id=10298
Member

WalterBright commented Jun 8, 2013

This bug needs fixing before this pull can work:

http://d.puremagic.com/issues/show_bug.cgi?id=10298
@yebblies

This comment has been minimized.

Show comment
Hide comment
@yebblies

yebblies Jun 8, 2013

Member

I'd rather do it this way and besides this way will work with all the backends.

Maybe so, but it still looks to me like doing it in the wrong place.
Are you sure the other backends don't support this already? @ibuclaw @klickverbot ?

Member

yebblies commented Jun 8, 2013

I'd rather do it this way and besides this way will work with all the backends.

Maybe so, but it still looks to me like doing it in the wrong place.
Are you sure the other backends don't support this already? @ibuclaw @klickverbot ?

@ibuclaw

This comment has been minimized.

Show comment
Hide comment
@ibuclaw

ibuclaw Jun 8, 2013

Member

GDC does this without needing optimisations turned on:

Code generated: s = S.init, *S.this (&s, 1);

Assembly (without -O)

movl    $3, -16(%rbp)
leaq    -24(%rbp), %rax
leaq    -16(%rbp), %rcx
movl    $1, %edx
movq    %rcx, %rsi
movq    %rax, %rdi
call    S.__ctor

Assembly (with -O)

movl    $3, %esi
Member

ibuclaw commented Jun 8, 2013

GDC does this without needing optimisations turned on:

Code generated: s = S.init, *S.this (&s, 1);

Assembly (without -O)

movl    $3, -16(%rbp)
leaq    -24(%rbp), %rax
leaq    -16(%rbp), %rcx
movl    $1, %edx
movq    %rcx, %rsi
movq    %rax, %rdi
call    S.__ctor

Assembly (with -O)

movl    $3, %esi

@WalterBright WalterBright referenced this pull request Jun 16, 2013

Merged

Fix null expression #2189

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 16, 2013

Member

This will continue to fail until #2189 is pulled.

Member

WalterBright commented Jun 16, 2013

This will continue to fail until #2189 is pulled.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 27, 2013

Member

added tweak to pass autotester

Member

WalterBright commented Jun 27, 2013

added tweak to pass autotester

@ibuclaw

This comment has been minimized.

Show comment
Hide comment
@ibuclaw

ibuclaw Jun 27, 2013

Member

While I congratulate this exposes bugs in CTFE - so we can improve that area, I'm not quite convinced about this. If you hold the following information in the backend:

  • whether the Symbol is read only (be it a global symbol or local stack var).
  • the static initialiser of the Symbol.

Then you be able to do this optimisation soley in the back end also. :)

Member

ibuclaw commented Jun 27, 2013

While I congratulate this exposes bugs in CTFE - so we can improve that area, I'm not quite convinced about this. If you hold the following information in the backend:

  • whether the Symbol is read only (be it a global symbol or local stack var).
  • the static initialiser of the Symbol.

Then you be able to do this optimisation soley in the back end also. :)

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Jun 28, 2013

Member

@ibuclaw you are correct, but this PR is a one liner with a significant positive effect, and doing the more general optimization, while worthwhile, is quite a bit more work and is further down the list of stuff I have to get to.

Member

WalterBright commented Jun 28, 2013

@ibuclaw you are correct, but this PR is a one liner with a significant positive effect, and doing the more general optimization, while worthwhile, is quite a bit more work and is further down the list of stuff I have to get to.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright
Member

WalterBright commented Jul 10, 2013

This has found another CTFE bug:

http://d.puremagic.com/issues/show_bug.cgi?id=10599

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Jan 27, 2015

Member

so what's the status of this

Member

andralex commented Jan 27, 2015

so what's the status of this

@ibuclaw

This comment has been minimized.

Show comment
Hide comment
@ibuclaw

ibuclaw Jan 30, 2015

Member

Looks like there are still CTFE bugs exposed because of this change.

Member

ibuclaw commented Jan 30, 2015

Looks like there are still CTFE bugs exposed because of this change.

@WalterBright

This comment has been minimized.

Show comment
Hide comment
@WalterBright

WalterBright Aug 23, 2015

Member

Rebased to ddmd

Member

WalterBright commented Aug 23, 2015

Rebased to ddmd

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex

andralex Jul 14, 2016

Member

@WalterBright rebase again

Member

andralex commented Jul 14, 2016

@WalterBright rebase again

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex
Member

andralex commented Jan 3, 2017

@andralex

This comment has been minimized.

Show comment
Hide comment
@andralex
Member

andralex commented Oct 15, 2017

@JinShil

This comment has been minimized.

Show comment
Hide comment
@JinShil

JinShil Nov 23, 2017

Member

Revived at #7359

Member

JinShil commented Nov 23, 2017

Revived at #7359

@JinShil JinShil closed this Nov 23, 2017

@JinShil

This comment has been minimized.

Show comment
Hide comment
@JinShil

JinShil Nov 25, 2017

Member

An issue was created at https://issues.dlang.org/show_bug.cgi?id=18009 to document the potential optimization that is the subject of this PR.

Member

JinShil commented Nov 25, 2017

An issue was created at https://issues.dlang.org/show_bug.cgi?id=18009 to document the potential optimization that is the subject of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment