Performance optimizations - #258
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e09012a63f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Visit all gotoStmt and labelStmt | ||
| for (const jp of Query.search(Joinpoint, { | ||
| for (const jp of Query.searchFrom($newNodes, Joinpoint, { |
There was a problem hiding this comment.
When the public API is used with a fresh Inliner for each call—as demonstrated in ClavaWeaver/resources/clava/test/api/InlinerTest.js:16-27—each instance starts labelNumber at zero. Since this traversal now sees only the detached $newNodes, it neither reserves nor renames labels inserted by earlier instances, so inlining two copies of a callee containing a_label into the same function emits two inliner_0_a_label: definitions and produces invalid C/C++. The generated name must be checked against labels in the destination function or allocated from state shared across instances.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid finding, and confirmed as a regression — and a correction to this reply's earlier wording, which wrongly said "the goldens are unchanged": the goldens ARE regenerated by this PR (numbering changed from the whole-AST counter churn to per-instance sequential); what was unchanged at the time of that reply was only the shared-instance test outcome.
Two additional points worth reporting:
-
The first attempt at the fix (checking generated names against Query.searchFrom($destFunction, LabelDecl)) did NOT work: LabelDecl join points are not discoverable through searchFrom, so the collision set was empty. This was caught empirically by adding the fresh-instance-per-call pattern from this comment's cited example to InlinerTest.js — the first run failed clang syntax validation with 'redefinition of label inliner_0_a_label', reproducing exactly the reported bug.
-
The fixed check collects existing labels from the destination function's goto/label statements (LabelDecl join points are attribute-like and only reachable via $jp.label/$jp.decl). With 518bfff, the new test case inlines the same labeled callee twice with a fresh Inliner per call and produces inliner_0_a_label / inliner_1_a_label, and clang syntax validation passes. Labels may still repeat across DIFFERENT functions (legal C, function-scoped), but never within one.
The added test case (callsFunctionWithLabelsFresh in inliner.c + the fresh-instance loop in InlinerTest.js) stays in the PR as a regression guard.
e09012a to
e08d307
Compare
lm-sousa
changed the title
e08d307 to
6b98c31
Compare
renameLabels() searched the whole AST for every inline even though the $newNodes parameter it received (unused since fa56385) already contains the complete copied body. Scope both searches with Query.searchFrom($newNodes, ...), which also stops renaming labels of unrelated functions and removes a latent replaceWith(undefined) on labels with no map entry. Regenerate InlinerTest goldens: label numbering is now per-inline sequential (inliner_0_, inliner_1_) instead of driven by the global whole-AST counter.
6b98c31 to
518bfff
Compare
|
1 participant
Stacked on #257 (only shows this commit until that merges).
Inliner.renameLabels()ran two whole-ASTQuery.search()passes for every inlined call — walking the entire program to rename labels that all live inside the copied body, even though the method already receives the complete $newNodes copy. The parameter has been unused since the feature's introduction (referenced only in a commented-outprintln), and the earlier triage attributed ~9 s of Query time to it.This scopes both searches with
Query.searchFrom($newNodes, …)— the same proven pattern the method already uses for forward declarations. Renaming runs on the detached body copy before insertion, where cross-function gotos cannot exist, so every goto/label pair stays consistently matched. As a side effect it stops renaming labels inside functions that were never inlined and eliminates a latentreplaceWith(undefined)on labels with no map entry; labels that clang's return-lowering attaches to the caller's own body now keep their original name, and each inlined copy carries its own renamed set, so no duplicate-label collision is possible.The InlinerTest goldens were regenerated: label numbering is now per-inline sequential (
inliner_0_,inliner_1_) instead of driven by the whole-AST counter; the diff touches label lines only.Part of the step-2 change set measured with the syntax-rebuild revisions: suite median 111.79 s → 47.60 s (the SpecsSystem half of that step is in specs-java-libs#32).