Optimistic Data-Flow Analysis Framework for Graal IR#13447
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
Bot
added
the
OCA Required
|
Thank you for signing the OCA. |
oracle-contributor-agreement
Bot
added
OCA Verified
gergo-
left a comment
gergo-
left a comment
There was a problem hiding this comment.
Thanks again for this PR, sorry for the long wait. I had a lot of comments about documentation/clarity/code organization. The actual code logic looks fine.
|
|
||
| /** | ||
| * This is an example where the return value can be found by the Parity10 analysis. Stamps do | ||
| * not cover if a value is divisible by 10, therefore this case here is not covered. |
There was a problem hiding this comment.
"... is not covered by other analyses."
| * It defines the domain that the analysis will use in the form of a | ||
| * <a href="https://en.wikipedia.org/wiki/Complete_lattice">complete lattice</a>. The elements | ||
| * in this domain are represented by instances of {@link ParityElement}. Domain definition | ||
| * instances are supposed to be essentially stateless. They only exist to provide functions to |
There was a problem hiding this comment.
"essentially"? Are there situations where the domain definition would not be stateless?
|
|
||
| /** | ||
| * This method calculates a result for the operation the given node represents, given the | ||
| * values of its inputs. |
There was a problem hiding this comment.
maybe something more like: "... given the mapping that contains the currently known information about its inputs"?
| yield ParityElement.L_TRUE; | ||
| } else if (x0 && y.isNotZero() || y0 && x.isNotZero()) { | ||
| yield ParityElement.L_FALSE; | ||
| } else { |
There was a problem hiding this comment.
Aren't there cases missing for "one input is parity 10, the other is not"?
| public ParityElement[][] calcInferrableValues(ValueNode node, DFAMap<ParityElement> map) { | ||
| if (node instanceof IntegerEqualsNode eq) { | ||
| return AnalysisDomainDefinition.ifInference(ParityElement.class, | ||
| // inferences for the true branch |
There was a problem hiding this comment.
Style note: I prefer /* block comments */ everywhere, including for single-line comments. I find it especially strange to mix the comment styles close together within one method, like here.
| assert !(b && doNotTripTwice) : "nondeterminism???"; | ||
| doNotTripTwice |= b; | ||
| } | ||
| assert doNotTripTwice : "we're on a road to nowhere"; |
There was a problem hiding this comment.
All the domains repeat the If and IntegerSwitch cases in essentially the same form, can you factor out the common patterns as helpers?
| /* | ||
| * Here we might encounter impossible values (at inferences in unreachable | ||
| * branches), which are canonicalized to UNEVALUATED. Since we should not | ||
| * infer UNEVALUATED inputs, we default to UNRESTRICTED. |
There was a problem hiding this comment.
Why are we building an inference at all, if it looks like we're on an impossible path?
| continue; | ||
| } | ||
| if ((evalCnt == widenAfter) && prelim instanceof IntegerPentagon iPtg) { | ||
| // special widening heuristic to catch non-overflowing counted loops |
There was a problem hiding this comment.
Please expand this comment a bit, explaining the strategy used. You can use numeric examples if that's helpful.
| if (x.contains(y)) { | ||
| return x; | ||
| } | ||
| Set<ValueNode> res = new HashSet<>(x == null ? Set.of() : x); |
There was a problem hiding this comment.
x is not null here, you have already dereferenced it. Similarly for some other cases.
| return !x.equals(y) && x.meet(y).equals(x); | ||
| } | ||
|
|
||
| private Pentagon normalize(Pentagon input) { |
gergo-
left a comment
gergo-
left a comment
There was a problem hiding this comment.
Thanks again for this PR, sorry for the long wait. I had a lot of comments about documentation/clarity/code organization. The actual code logic looks fine.
2 participants
Summary
This pull request adds a framework for optimistic data-flow analysis in Graal IR.
Furthermore, three additional mid-tier phases are provided, a Lazy Sparse Conditional Constant Propagation phase (LSCCP paper), a Full Stamp Analysis phase, and a Pentagonal Analysis Phase (Pentagons paper). All of these phases perform their respective analysis on the Graal IR graph and then replace nodes found to be constant. Unreachable branches are subsequently removed using the canonicalizer. By default, only the LSCCP phase is enabled.
Related Issues
Testing
This PR features a host of tests, which either check for capabilities of the three analyses, or which cover specific problems with the framework experienced throughout development.
As a demo of how to use the framework, an additional test implementing an analysis, checking if an integer value is divisible by 10. This test is extensively commented to show how to implement an analysis without needing to go into detail on the inner workings of the framework.
Documentation
Contributor Checklist