Skip to content

Conversation

@limepoutine
Copy link
Contributor

Fix #21974. Let's see what havoc it can wreak on CI.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @limepoutine! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21977"

@rainers
Copy link
Member

rainers commented Oct 15, 2025

LGTM. Can you add a test? One option might be to add your reported test case, which should link without the imported data.d.

This reveals that pragma(inline, true) doesn't force the inlining yet (even without -inline). This happens because main is not marked hasAlwaysInline and the FuncExp is never seen by the visitor as referenced by main. Is it possible to add the function to the module members earlier?

@limepoutine
Copy link
Contributor Author

One option might be to add your reported test case, which should link without the imported data.d.

Done.

Is it possible to add the function to the module members earlier?

Nested functions are added to the parent's localsymtab, not module members. I think I can visit all local symbols in InlineScanVisitorDsymbol, but that both wastes cycles (target function might have been eliminated by CTFE) and makes ordering issues worse.

@rainers
Copy link
Member

rainers commented Oct 16, 2025

Done.

Thanks.

Nested functions are added to the parent's localsymtab, not module members. I think I can visit all local symbols in InlineScanVisitorDsymbol, but that both wastes cycles (target function might have been eliminated by CTFE) and makes ordering issues worse.

I tried filling an extra array in the module with literals that are calling functions with pragma(inline, true), but I also don't like that this doesn't handle eliminated functions well. So walking the local symbol table might be the better alternative (does that need to walk inner scopes, too? how are regular nested functions processed now?). AFAICT a function that has skipCodegen set doesn't need to be checked for inlining at all, including its nested functions.
Adding the function literals to the module members might be unexpected indeed, but I think there are no ordering issues when processing inlining.

@rainers
Copy link
Member

rainers commented Oct 16, 2025

ATM pragma(inline, true) is currently more or less ignored in nested functions, so we can defer fixing that to another PR.

@ibuclaw
Copy link
Member

ibuclaw commented Oct 16, 2025

Do note the frontend inliner is half deprecated in dmd, I guess ignored in ldc and nonexistent in gdc. Effort on getting the backend inliner sorted might be better collective use of time.

@limepoutine
Copy link
Contributor Author

Previously, named nested functions are scanned when visiting VarDeclaration (are their parents consistently marked hasAlwaysInline in semantic3? I remember not always) and unnamed ones completely handled ad hoc (hence this issue). #21983 removes these cases and always walks the local symbol table.

The backend inliner is too half-baked right now and tracking hasAlwaysInline across the glue layer seems rather painful... At minimum this amounts to a bit of refactor in the frontend inliner?

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.

Mixin functions in foreach body silently fails to inline

4 participants