ConstraintAnalysis: Parse tees - #9109
Conversation
| push_back(get); | ||
| return LocalOperation{get->index, get->type}; | ||
| } | ||
| if (auto* set = curr->dynCast<LocalSet>()) { |
There was a problem hiding this comment.
Can we add an assertion that curr is a tee here?
| // Insert a read, because the tee does both a write and a read. | ||
| read.insert(set->index); |
There was a problem hiding this comment.
Should this be guarded behind a check that the set is actually a tee? If a nested expression contains a block, we might have sets that are not tees.
| // common cases we want to, we parse the code in the natural order of execution, | ||
| // and maintain a list of local operations. A get before a tee indicates | ||
| // possible interference. | ||
| struct LocalOperations : public SmallVector<Expression*, 10> { |
There was a problem hiding this comment.
I don't think it's worth extending SmallVector here. It would be simple enough and clearer to have the vector as a normal member.
|
|
||
| // Check for any possible interference between locals, which would tell the | ||
| // caller that whatever was parsed is not valid. | ||
| bool hasLocalInterference() const { |
There was a problem hiding this comment.
Instead of keeping a vector of local reads and writes, can we just keep the vector of reads (including tees) and determine whether there is interference online as we see more accesses? I don't see a reason to defer this analysis to a separate step.
There was a problem hiding this comment.
Deferring it avoids creating a std::unordered_set in the vast majority of cases, as most code has no tees at all, and it avoids writing to it in the common case of just one tee.
| // Canonicalize EqZ to Eq of 0. | ||
| auto value = Literal::makeZero(get->type); | ||
| return LocalConstraint{get->index, Constraint{Abstract::Eq, {value}}}; | ||
| auto value = Literal::makeZero(localOp->type); |
There was a problem hiding this comment.
Can the type ever be anything other than the type of value? If not, we can simplify LocalOperation by not storing the type.
There was a problem hiding this comment.
Unfortunately, the type of a tee matches the local, not the value
There was a problem hiding this comment.
That probably won't be a problem for Literal::makeZero, though, right? The type of the tee's value must be a subtype of the tee's type, so they will have the same zero value.
There was a problem hiding this comment.
Fair enough, done. It feels slightly wrong to be imprecise here, but yeah, this is just for nulls...
2 participants
Parsing tees allows us to handle more code, as it is common to see
a tee at the start of a constraint, e.g.
This is also a bugfix, as we were not handling unparsed tees before:
we need to make sure that no tee tramples a get that we parse into
a constraint. E.g.
We cannot parse that into
$x && ..because at the AND, we have alreadytrampled
$x. This code looks for any such conflict.