Skip to content

C#: Use CFG nodes instead of AST nodes in sign/modulus analysis - #4445

Merged
hvitved merged 3 commits into
github:mainfrom
hvitved:csharp/sign-analysis-cfg
Oct 26, 2020
Merged

C#: Use CFG nodes instead of AST nodes in sign/modulus analysis#4445
hvitved merged 3 commits into
github:mainfrom
hvitved:csharp/sign-analysis-cfg

Conversation

@hvitved

@hvitved hvitved commented Oct 9, 2020

Copy link
Copy Markdown
Contributor

This PR rewrites sign/modulus analysis for C# to use CFG nodes instead of AST nodes, in order to take CFG splitting into account. For example, in

void Splitting2(bool b)
{
    int x;
    if (b) x = 1; else x = -1;

    System.Console.WriteLine(x); // strictly positive (b = true) or strictly negative (b = false)

    if (b)
        System.Console.WriteLine(x); // strictly positive
    else
        System.Console.WriteLine(x); // strictly negative
}

x exists in two copies in the CFG at the first System.Console.WriteLine(x), and the new analysis is able to deduce that it is strictly positive in the copy for b = true and strictly negative in the copy for b = false.

The implementation uses (a copy of) the ControlFlowReachability library that we also use to take CFG splitting into account in the data-flow library.

I had to make two changes in the shared interfaces:

  • rankedPhiInput is now moved out into the language-specific modules, as we need to rank on two columns for C#. It turned out that the old implementation was partial, which resulted in diverging analysis for loops at the beginning of a callable. The new (total) implementation is based on a similar pattern from the basic block library.
  • I added a getSourceType predicate to CastExpr.

@hvitved
hvitved force-pushed the csharp/sign-analysis-cfg branch 2 times, most recently from f4cf83e to 5ad29c8 Compare October 13, 2020 13:50
@hvitved
hvitved force-pushed the csharp/sign-analysis-cfg branch 3 times, most recently from fdaee7d to de6b13a Compare October 14, 2020 10:18
@hvitved
hvitved force-pushed the csharp/sign-analysis-cfg branch from de6b13a to 2af7e1c Compare October 14, 2020 11:39
@hvitved
hvitved marked this pull request as ready for review October 14, 2020 11:44
@hvitved
hvitved requested review from a team as code owners October 14, 2020 11:44
@aschackmull

Copy link
Copy Markdown
Contributor

Java changes LGTM.

@tamasvajk tamasvajk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, we'll see with RangeAnalysis if I understood everything in the PR. :-)

@hvitved
hvitved merged commit 492b114 into github:main Oct 26, 2020
@hvitved
hvitved deleted the csharp/sign-analysis-cfg branch October 26, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants