Make IR DAG into tree to remove ambiguities #2388
Conversation
backends/bmv2/common/lower.h
Outdated
|
|
||
| namespace BMV2 { | ||
|
|
||
| /** | ||
| This pass rewrites expressions which are not supported natively on BMv2. | ||
| */ | ||
| class LowerExpressions : public Transform { |
There was a problem hiding this comment.
Seems like this is actually a bug in some later pass that can't handle DAGs properly. We should at least document which one(s), nothing where they fail when expression nodes appear in mulitple places in the DAG and why.
There was a problem hiding this comment.
The translation to JSON uses a hashtable to map an IR node to its translation.
Potentially all passes that do something similar are vulnerable.
There was a problem hiding this comment.
If the translation to JSON of a node depends on the parent context in which the node appears, then it shouldn't do that -- its a bug. Other passes that do similar things are also buggy -- we should at least document all the potentially unsafe assumptions that passes make.
There was a problem hiding this comment.
Reviewing the code for all passes would take a significant amount of time, so I am not sure when this could happen. In principle any time you use a set or map with a node as a key you have to worry about this, and there are a lot of cases. If we had more help with the front-end development I would consider doing this, but at this point I don't have enough time.
Unfortunately I didn't really internalize clearly the consequences of sharing nodes; even very early code (2015) that was written assumes sometimes implicitly that the IR DAG is a tree. Ideally we should have caught this in code review at the time.
There was a problem hiding this comment.
It also doesn't help that writing tests for these conditions is very difficult.
There was a problem hiding this comment.
This might defeat the purpose of the hashtable mapping IR nodes to other data, but would it be correct if instead it mapped a path of IR nodes, from a "root" to the IR node in question, to that data, instead of only the last IR node on that path?
There was a problem hiding this comment.
We do have a data structure for that, it's called a "context", but it exists only while you are traversing in a visitor. You would need to copy it separately and add equality tests... quite a bit of work.
There was a problem hiding this comment.
@mbudiu-vmw Please consider taking a small (size of code) frontend pass and make changes to it akin to this PR. Then, the community could help with frontend code and also think of how to test. thanks.
There was a problem hiding this comment.
I don't understand this request.
Regarding @jafingerhut's question: it is not clear that you want equality testing for the path; you may still treat two nodes the same even if they have different paths; for example many passes treat left-values different from the other expressions, but similar to each other, no matter where they show up in the tree. So I don't know if such a data structure would actually solve all the problems.
There was a problem hiding this comment.
I don't understand this request.
Since you said "any time you use a set or map with a node as a key you have to worry about this, and there are a lot of cases.", I meant, why not look at std::map and std::set use in one frontend pass and see how to fix the issue.
|
Please fix this phrase in the documentation: "// WARNING: this assumes that each the same IR node has the same" |
|
Fixes #2390 |
The problem here is that the IR DAG has the same expression object in the LHS and RHS, but the code generated needs to be different.
This also fixes another clang warning.
Fixes #2375