Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Oct 9, 2020

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
Contributor

Java changes LGTM.

Copy link
Contributor

@tamasvajk tamasvajk left a comment

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