Fix for issue #270 #271
Fix for issue #270 #271
Conversation
backends/bmv2/lower.cpp
Outdated
| auto result = new VariableDefinitions(); | ||
| result->writers = writers; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Seems cleaner to use the copy ctor here, so the body would be just
return new VariableDefinitions(*this);
backends/bmv2/lower.cpp
Outdated
| auto result = clone(); | ||
| for (auto e : other->writers) | ||
| result->writers.emplace(e); | ||
| return result; |
There was a problem hiding this comment.
If e.first (the string) already has an entry in writers, then this won't actually do anything -- shouldn't it be unioning the sets in this case? Or can it never happen (should probably add a BUG_CHECK if so)
There was a problem hiding this comment.
This is a bug, I will submit a patch.
| // analysis is run. We take advantage that some more complex code | ||
| // patterns have already been eliminated. | ||
| class Accesses : public Inspector { | ||
| PathSubstitutions* substitutions; |
There was a problem hiding this comment.
Using ControlFlowVisitor here would make the special handling of IfStatement (and other control flow constructs) below unnecessary. Using P4WriteContext would obviate the need for the special handling of assignments, as well as make the code (potentially) work for out params of method calls.
There was a problem hiding this comment.
You are right about this; however, we only have 1 example using P4WriteContext in the compiler, and it is currently not working correctly - this is issue #242. So I have resorted to using a more explicit approach.
I am also relying on the fact that on this architecture you cannot call methods with out arguments in the checksum verification stage.
backends/bmv2/lower.cpp
Outdated
| if (prev == writers.end()) | ||
| result->writers[e.first] = e.second; | ||
| else | ||
| result->writers[e.first] = prev->second->join(e.second); |
There was a problem hiding this comment.
This could be just
auto &prev = result->writers[e.first];
prev = prev ? prev->join(e.second) : e.second;
could be even simpler if you didn't have the (unnecessary?) extra level of pointer in the map
This is reworking an older fix which proved to be incomplete; this code does some copy propagation only in the control which does checksum updates. It takes advantage of the fact that we only allow very restricted code in this control - the only method that can be called is the checksum.get() method.
The code can now handle multiple checksum units. It is not foolproof; some hand-written code patterns can trip it - the foolproof solution should probably use an extern in the BMv2 simulator for checksum calculations.
(I really don't know why the changes to the tableKeyNames.cpp file are in this PR; I should have done a rebase.)