Ssa: Refactor shared SSA in preparation for eliminating phi-read definitions #18819
Conversation
This yields a tiny bit of additional tuples consistent with the prior Java implementation.
aschackmull
added
the
no-change-note-required
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This is a result of the bugfix in the commit named "C#/Ruby/Rust: Fix bug in adjacentReadPairSameVar"
shared/ssa/codeql/ssa/Ssa.qll
Outdated
| * Holds if we should synthesize a pseudo-read of `v` at the beginning of `bb`. | ||
| * | ||
| * These reads are named phi-reads, since they are constructed in the same | ||
| * way as ordinary phi nodes except all all reads are treated as potential |
shared/ssa/codeql/ssa/Ssa.qll
Outdated
| variableWrite(bb, i, v, false) and | ||
| k = SsaActualRead() |
There was a problem hiding this comment.
Why is it that we have "pseudo-reads associated with uncertain writes"?
There was a problem hiding this comment.
That makes it much simpler to calculate uncertainWriteDefinitionInput.
| BasicBlock bb1, int i1, BasicBlock bb2, int i2, SourceVariable v, boolean samevar | ||
| ) { | ||
| adjacentRefs(bb1, i1, bb2, i2, v) and | ||
| variableRead(bb2, i2, v, _) and |
There was a problem hiding this comment.
The QL doc mentions certain read, so should _ be true?
There was a problem hiding this comment.
That should be implied and hence an unnecessary join due to the restriction in ssaRef to only include certain reads.
| variableWrite(bb, i, v, false) and | ||
| k = Read() |
There was a problem hiding this comment.
I understand now, per your comment, why this case is needed for uncertainWriteDefinitionInput in SsaDefReachesNew. Why is it needed here?
There was a problem hiding this comment.
Once we've added phi-read nodes, the use-use calculation can be done in exactly the same way as def-use: Any non-phi reference will be dominated by the prior reference. So for actual reads and uncertain writes there exist a unique previous reference, which we calculate in adjacentRefRead, and including k = Read() for uncertain writes make it easy to include those cases that way in adjacentRefRead. And note that we subsequently need those when we calculate the samevar=false flavour of use-use pairs in firstUseAfterRef, which exactly skips forward over references that aren't actual certain reads.
I want to reinterpret phi-reads as reads rather than definitions and hide them from the exposed interface of the SSA library. For normal SSA usage they should be irrelevant: they're only useful for getting an optimised use-use step relation, in particular for use in data-flow.
This PR leaves most of the existing implementation intact, but adds alternative predicates and switches the internal implementation of several others in preparation for this change.
The PR is structured as a long sequence of individually simple commits, so commit-by-commit review is very much encouraged.
There's more work to come, but each of the commits should make sense on their own, and I'd gathered so many that I wanted to merge them to main before adding too many more.