[Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause#9569
Conversation
electriclilies
requested review from
MarisaKirisame,
ZihengJiang,
anijain2305,
icemelon,
jroesch,
junrushao,
jwfromm,
mbrookhart,
slyubomirsky,
vinx13,
wweic,
yzhliu and
zhiics
as code owners
mikepapadim
left a comment
mikepapadim
left a comment
There was a problem hiding this comment.
LGTM, maybe add a cpp tests for new Clause.
| } | ||
| // Note: The partial evaluator seems to do some weird stuff with sharing. Changing Tuple(expr) | ||
| // to WithFields(op, expr) causes some strange failures. | ||
| return HasStatic(MkSTuple(value), ll->Push(Tuple(expr))); |
There was a problem hiding this comment.
Can we get capture these failures with some unit-test?
There was a problem hiding this comment.
I was seeing some failures in the partial evaluator tests, but also in other places. I'll update the comment to clarify, I don't think I need to add another unittest here though
| Expr tuple = opt_tuple.value_or(tuple_get_item->tuple); | ||
| int index = | ||
| opt_index.value_or(tuple_get_item->index) | ||
| ->value; // Is it OK that this is an Integer instead of int? this lets me use Optional |
There was a problem hiding this comment.
I think Integer is fine here. Still need this comment?
There was a problem hiding this comment.
nice catch, thanks!
mbs-octoml
left a comment
mbs-octoml
left a comment
There was a problem hiding this comment.
Look great, thank you.
For bonus points you could replace the explicit ctors in device_planner.cc with WithField.
| Optional<Array<Expr>> opt_args = Optional<Array<Expr>>(), | ||
| Optional<Attrs> opt_attrs = Optional<Attrs>(), | ||
| Optional<Array<Type>> opt_type_args = Optional<Array<Type>>(), | ||
| Optional<Span> opt_span = Optional<Span>(nullptr)); |
There was a problem hiding this comment.
don't need the nullptr arg apparently
jroesch
left a comment
jroesch
left a comment
There was a problem hiding this comment.
Changes look great, thanks Lily! Just minor question about moving const references.
| return Clause(pat, VisitExpr(c->rhs)); | ||
| Clause VisitClause(const Clause& clause) final { | ||
| Pattern lhs = VisitPattern(clause->lhs); | ||
| return WithFields(std::move(clause), std::move(lhs), VisitExpr(clause->rhs)); |
There was a problem hiding this comment.
@mbs-octoml I don't know if we actually need these moves, for const reference does this matter? I can't actually remember what the spec says about value vs move semantics for const &.
There was a problem hiding this comment.
Oh, didn't even notice it was const. I was just std::moving everything that was named that we were not using again
|
@mbs-octoml Changed the explicit constructions in the device planner to copy except for one Tuple, since I don't have WithFields for tuples in this PR (waiting on #9533). |
electriclilies
force-pushed
the
with_fields
branch
from
d8a6114 to
68dedb4
Compare
electriclilies
force-pushed
the
with_fields
branch
from
68dedb4 to
b58c83c
Compare
|
@Mousius or @manupa-arm could you merge? It’s a holiday weekend here and I’d like to get this in before there are any merge conflicts. Thanks! |
manupak
left a comment
manupak
left a comment
There was a problem hiding this comment.
LGTM. Getting this in for now.
Lets follow up if needed
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
…Let, RefCreate, RefRead, RefWrite, Match, and Clause (apache#9569) * Implement WithFields for Relay exprs * lint
5 participants
In this PR, I implement the WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match and Clause. It is a continuation of work in #9533, where I implement WithFields for Tuples.
Additionally, I remove the explicit COW logic from the visitors in expr_functor.cc for these classes. In future work, I'll use these methods to replace most explicit reconstructions of these classes in visitors.
@mbs-octoml @mikepapadim PTAL! I would especially appreciate typo checks in the documentation, I did a lot of copy-pasting and probably missed some things there.
cc @tqchen in case you are interested -- I've kept the API consistent with what we discussed for WithFields for tuples