Skip to content
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

eliminate redundant inline scanning #5117

Merged
merged 1 commit into from
Sep 25, 2015
Merged

Conversation

WalterBright
Copy link
Member

While working on a fix to do better delegate inlining, I discovered that the inliner is constantly rescanning functions for inline possibilities. Added a flag FUNCFLAGinlineScanned to put a stop to that, and a flag again to enable rescanning in case of delegate parameters.

The latter isn't complete yet, but this PR won't hurt.

FuncDeclaration oldparent = parent;
parent = fd;
if (fd.fbody && !fd.naked)
if (fd.fbody && !fd.naked)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

@@ -393,6 +393,7 @@ enum FUNCFLAGsafetyInprocess = 2; // working on determining safety
enum FUNCFLAGnothrowInprocess = 4; // working on determining nothrow
enum FUNCFLAGnogcInprocess = 8; // working on determining @nogc
enum FUNCFLAGreturnInprocess = 0x10; // working on inferring 'return' for parameters
enum FUNCFLAGinlineScanned = 0x20; // function has been scanned for inline possibilities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/inline possibilities/inlining opportunities/ I guess but no matter

@9rnsr
Copy link
Contributor

9rnsr commented Sep 25, 2015

Looks good to me.

@andralex
Copy link
Member

Auto-merge toggled on

andralex added a commit that referenced this pull request Sep 25, 2015
eliminate redundant inline scanning
@andralex andralex merged commit c5ee583 into dlang:master Sep 25, 2015
@WalterBright WalterBright deleted the inlineagain branch September 26, 2015 00:16
@9rnsr
Copy link
Contributor

9rnsr commented Oct 30, 2015

This PR introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=15253

@9rnsr
Copy link
Contributor

9rnsr commented Oct 30, 2015

I have a question about this change.

Q1. To see the inline possibility of a function, InlineScanVisitor.visit(FuncDeclaration) will be called:

  1. from inlineScanModule - it's invoked in the global "inlining" stage. In this case, InlineScanVisitor.visit(FuncDeclaration) will be called only once for each functions.

  2. from canInline - it's called from each expression/statement inlining, so it could be called during expandInline process. However in it, fd.inlineNest is already tested and rejecting recursive scanning, so InlineScanVisitor.visit(FuncDeclaration) won't be called recursively.
    And also, the scanning result is cached into fd.inlineStatus(Stmt|Exp) field, so repeated scanning is not also there.

    Therefore, I cannot see why we need to have a new flag FUNCFLAGinlineScanned. To me it's just a duplication of fd.inlineNest.

Q2. As I already specified in Q1, recursive expansion is tested and prevented by canInline. If we do repeated scanning for more inline possibility, the again flag seems to be set in canInline function, IMO.

@9rnsr
Copy link
Contributor

9rnsr commented Oct 30, 2015

Ah, if InlineScanVisitor.visit(FuncDeclaration) is called from inlineScanModule, it would ignore the cached result fd.inlineStatusStmt or fd.inlineStatusExp.
And if InlineScanVisitor.visit(FuncDeclaration) is called from canInline, it would ignore the previous scan result that is calculated via inlineScanModule.

In other words, a function will be scanned at most three times - 1. as function declaration, 2. as a statement, and 3. as an expression. Before this change, the 2. result is cached to fd.inlineStatusStmt, and 3. is cached to fd.inlineStatusExp. After this change the 1. result is cached to fd.flags & FUNCFLAGinlineScanned.

But, as I already said in Q1, the No.1 case == InlineScanVisitor.visit(FuncDeclaration) call from inlineScanModule would happen at most once. I still think caching its result would not be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants