Bound #521
Bound #521
Conversation
| @@ -601,6 +601,8 @@ class IndexStmt : public util::IntrusivePtr<const IndexStmtNode> { | |||
| /// variable, a \textit{tail strategy} is employed such as emitting a variable | |||
| /// sized loop that handles remaining iterations. | |||
| /// Preconditions: splitFactor is a positive nonzero integer | |||
| IndexStmt splitUpDown(IndexVar i, IndexVar i1, IndexVar i2, bool split_up, size_t splitFactor) const; | |||
There was a problem hiding this comment.
It feels like this shouldn't be a separate API call, but instead an additional argument to the existing split call that takes in an enum, with an enum that has a default value.
There was a problem hiding this comment.
Removed: it is not relevant to this pull request
| IndexStmt bound(IndexVar i, IndexVar i1, size_t bound, BoundType bound_type) const; | ||
|
|
||
|
|
||
| IndexStmt bound(IndexVar i, size_t bound, BoundType bound_type) const; |
There was a problem hiding this comment.
You can probably remove the old bound call and replace all users with the new method (I don't think we care that much about backwards compat, and bound doesn't have that many users).
| @@ -1031,6 +1038,18 @@ class IndexVar : public IndexExpr, public IndexVarInterface { | |||
| /// Returns the name of the index variable. | |||
| std::string getName() const; | |||
|
|
|||
| size_t& getBound() const; | |||
There was a problem hiding this comment.
Why are these things attached to this index variable instead of tagged to a particular for loop?
There was a problem hiding this comment.
Mapping a for all to an index var's bound is not possible since adding schedules replaces the occurrence of the index var from the stmt in certain cases.
Instead, in the updated pull request, we tag this information to a suchThatNode, adding a map that relates the bounded variable, bound size and bound type.
| if (!transformed.defined()) { | ||
| taco_uerror << reason; | ||
| } | ||
| taco_uerror << "Depericated Bound: bounding " << i.getName() << " ignoring " << i1.getName() << endl; |
There was a problem hiding this comment.
spelling error, but this method should be removed.
| if (indexVar == getOuterVar()) { | ||
| ir::Expr minBound = ir::Div::make(parentBound[0], ir::Literal::make(getSplitFactor(), splitFactorType)); | ||
| ir::Expr maxBound = ir::Div::make(ir::Add::make(parentBound[1], ir::Literal::make(getSplitFactor()-1, splitFactorType)), ir::Literal::make(getSplitFactor(), splitFactorType)); | ||
| if ( ir::isa<ir::Literal>(ir::simplify(maxBound)) && indexVar.isBound() && !ir::to<ir::Literal>(ir::simplify(maxBound))->equalsScalar(indexVar.getBound()) ){ |
There was a problem hiding this comment.
I think this a reasonable restriction, and can instead be checked when performing the split transformation rather than here.
| @@ -461,6 +473,23 @@ std::vector<ir::Expr> PosRelNode::deriveIterBounds(taco::IndexVar indexVar, | |||
| std::map<taco::IndexVar, taco::ir::Expr> variableNames, | |||
| Iterators iterators, | |||
| ProvenanceGraph provGraph) const { | |||
|
|
|||
|
|
|||
| if (indexVar.isBound()){ | |||
There was a problem hiding this comment.
If the provenance graph still has to answer queries about bounded variables, then it should have the information about bounded variables, rather than the indexvar perhaps. I could see a relation similar to how bound existed already, but is instead a marker for an index variable, rather than relating two variables? I assume that this was considered in your discussions though.
There was a problem hiding this comment.
In the updated pull request, the provenance graph keeps a map of bounded vars. We cannot add the bound as VarRelNode instead since we want to get around the problem of having to create a new indexVar in order to bound it.
At it's current implementation, we cannot have cycles in the provenance graph, but the bound (if implemented as a RelNode, we will introduce a cycle since the parent and child would be the same).
| taco_iassert(indexVar == getPrecomputeVar()); | ||
| taco_iassert(parentIterBounds.count(getParentVar()) == 1); | ||
|
|
||
| if (indexVar.isBound()){ |
There was a problem hiding this comment.
It's quite weird that each relation node's implementation now has to check things about bound. That's not a modular design
There was a problem hiding this comment.
Changed to remove this check for deriveIterBounds from the RelNodes functions to the deriveIterBounds for the provenance graph (so, we only need to make this check once).
Remove bound as a relation node in the provenance graph. Instead, tag the indexVar with information about the bound.