C++: Include "phi reads" in DataFlow::Node
#12356
Conversation
|
Hmmm... I don't understand why we gain so many new results from this, but they look like TPs to me 😮. The two results on |
You also fixed a bug. Does that have something to do with it? |
Good point. I had the exact same thought, so I actually made a PR with just that fix here and ran a DCA on that, but as you can see from the DCA run, that did not actually produce any new results. |
|
I may understand part of the issue now. I've found the step that's responsible for the new results on Notice that The reason I'm saying tht I may understand part of the issue now is that I can't seem to reproduce the above in a local test file 😕. So there may be something else going on. Now, am I happy with the fact that this is how we're getting the flow? No, not really 😂. But we would have gotten this result anyway once we implement proper support for static locals, so I don't really mind the fact that we're getting this result now. This explains the two new results on |
|
I'm seeing the exact same situation on the new |
There's a small fix to the mapping from 'global def -> use'. Finally, this commit also accepts a test failure related to new missing types for phi nodes. The fix for that is in the next commit.
|
I manage to reproduce the above situation in a QL test 🎉. I've rebased the PR to make it follow the usual "add test", "write QL", "accept tests" workflow. No actual QL code has changed in the force-push! I've looked through a couple of the new results on |
| exists(PhiNode phi | | ||
| lastRefRedefExt(_, bb1, i1, phi) and | ||
| useOrPhi.asPhi() = phi and | ||
| phi.getSourceVariable() = pragma[only_bind_into](v) |
There was a problem hiding this comment.
PR mostly looks sensible. This is the only part I have trouble wrapping my head around. What does this do?
There was a problem hiding this comment.
lastRefRedefExt(_, bb1, i1, phi) says that the last reference to the variable defined by phi was at index i1 in basic block bb1. This means that (bb1, i1) is an input to phi. Does that help?
It's basically the same pattern as here. We should probably factor that out into a common predicate in another PR.
MathiasVP
merged commit 16e817c
into
github:mathiasvp/replace-ast-with-ir-use-usedataflow
In #11198 Tom added phi nodes for uses in the shared SSA library, but we haven't yet reaped the benefit of those in C++ since we've been using the backwards compatibility wrappers he made. This PR makes use of those new phi nodes for uses. It should give us a small performance boost 🤞.
The first commit does the real work, and the second commit fixes a bug exposed by the tests.