Make sure P4 expression optimization does not strip away types #4300
Conversation
vlstill
changed the title
vlstill
added
bug
vlstill
force-pushed
the
fix-state-types
branch
from
4353b86
to
bf4e8cc
Compare
| @@ -37,7 +37,9 @@ std::vector<std::pair<IR::StateVariable, const IR::Expression *>> ExprStepper::s | |||
| ExecutionState &nextState, const std::vector<IR::StateVariable> &flatFields, | |||
| int varBitFieldSize) { | |||
| std::vector<std::pair<IR::StateVariable, const IR::Expression *>> fields; | |||
| for (const auto &fieldRef : flatFields) { | |||
| // Make a copy of the StateVariable so it can be modified in the varbit case (and it is just a | |||
There was a problem hiding this comment.
Hmm, in this case we are making a copy for the common case (no Varbit). Is that really better?
There was a problem hiding this comment.
This is quite small class (one pointer + vptr) so I'd say this does not matter either way. Could be even better since there will be no dereference, but could be also exactly the same if the optimizer is smart enough (I haven't checked).
There was a problem hiding this comment.
Yeah I think that is a check we should keep in this particular setter. The other code can be moved out.
There was a problem hiding this comment.
I'm confused, you have presubmably replied to a different thread, did you mean that the check for type being set in the expression should be (also) in SymbolicEnv::set?
There was a problem hiding this comment.
Yes, the comment was meant for #4300 (comment). Not sure what happened.
vlstill
force-pushed
the
fix-state-types
branch
from
fc56757
to
9264808
Compare
| @@ -28,7 +27,7 @@ const IR::Expression *SymbolicEnv::get(const IR::StateVariable &var) const { | |||
| bool SymbolicEnv::exists(const IR::StateVariable &var) const { return map.find(var) != map.end(); } | |||
|
|
|||
| void SymbolicEnv::set(const IR::StateVariable &var, const IR::Expression *value) { | |||
| map[var] = P4::optimizeExpression(value); | |||
There was a problem hiding this comment.
If we remove the optimization, I'm not sure if the check if type is set should be here too? May not be necessary for all tools that use SymbolicEnv.
vlstill
force-pushed
the
fix-state-types
branch
from
9264808
to
1557806
Compare
| @@ -37,7 +37,9 @@ std::vector<std::pair<IR::StateVariable, const IR::Expression *>> ExprStepper::s | |||
| ExecutionState &nextState, const std::vector<IR::StateVariable> &flatFields, | |||
| int varBitFieldSize) { | |||
| std::vector<std::pair<IR::StateVariable, const IR::Expression *>> fields; | |||
| for (const auto &fieldRef : flatFields) { | |||
| // Make a copy of the StateVariable so it can be modified in the varbit case (and it is just a | |||
There was a problem hiding this comment.
Yeah I think that is a check we should keep in this particular setter. The other code can be moved out.
vlstill
force-pushed
the
fix-state-types
branch
from
1557806
to
fc66333
Compare
Fixes this error:
Which is caused by
P4::optimizeExpressionnot preserving type when changingx - CONSTtox + (-CONST).The change is mainly in two passes: ConstantFolding and StrengthReduction -- I've tried to make sure the types of expressions are preserved by these passes. I've also added propagation of source info to a few places.