Skip to content

C++: Map some indirect nodes to expressions in localExprFlowStep #12507

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

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Mar 13, 2023

This PR supersede #12477 and (hopefully) contains the correct performance fix.

On certain projects containing heavily macro-expanded code like:

*x = source();
use(x[0]);
use(x[1]);
...
use(x[i]);
use(x[i+1]);
...
use(x[N]);

we hit a performance problem caused by each expression of the form x[i] being stepped to from *x = source() in certain cases in the expression-only local flow relation.

To see why this is the case, consider the transitive closure over localStepFromNonExpr in localStepsToExpr. We start at n2 for which n.asExpr() exists. For example, n2 in the above example could be x[i] in any of the use(x[i]) above. We then step to a dataflow predecessor of n2. In the above code fragment, thats the indirect node corresponding to x in x[i-1]. Since this doesn't have a result for Node::asExpr() we continue with the recursion until we reach *x = source() which does have a result for Node::asExpr().

If N is very large this blows up.

This PR fixes this problem by mapping those indirect nodes for x in x[i] to the x[i] expression. Ideally, this would be handled automatically by assigning the dataflow node for the indirection of x to the same dataflow node as x[i], but doing this in the past has let to some strange conflations. However, by just doing this in the expression-only local flow relation we're not seeing these strange conflations.

@MathiasVP MathiasVP requested a review from a team as a code owner March 13, 2023 13:37
@github-actions github-actions bot added the C++ label Mar 13, 2023
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Mar 13, 2023
Comment on lines 1144 to +1146
private class IndirectInstructionIndirectExprNode extends IndirectExprNodeBase,
RawIndirectInstruction {
RawIndirectInstruction
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like in the previous version of this PR, this change is the result of me running the new autoformatter on this file which hasn't been updated to include the latest autoformatter changes.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM if DCA is happy.

@MathiasVP MathiasVP merged commit 136769d into github:mathiasvp/replace-ast-with-ir-use-usedataflow Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants