[Relay] Partial Evaluator do concatenate, and has better termination checker for scalar. #3703
Conversation
|
Thanks for the PR @MarisaKirisame, could you fix the title to "[Relay] PE inline scalar" and in the description spell out "Partial evaluation" so people can get context? |
|
@tmoreau89 thx! It is done. |
There was a problem hiding this comment.
@MarisaKirisame This PR looks sick!
I think you forgot to change the title to "Partial evaluation" instead of "PE". Also, it looks like there's more than just scalar inlining in here. A more descriptive title might be "[Relay] Extend coverage of partial evaluation".
src/relay/pass/partial_eval.cc
Outdated
| return Fuel(make_node<FTValueNode>(tvalue)); | ||
| } | ||
|
|
||
| /*! \brief Initially every element has Fuel of FBottom. It is the smallest element. |
There was a problem hiding this comment.
I don't understand the code or lattices very well yet, but if every element starts at FBottom, and you only do meets, then wouldn't it remain FBottom forever, since the meet of a minimal element and any other element is always the minimal element?
If that's the case, then maybe this should be FTop?
There was a problem hiding this comment.
Good catch! It is indeed FTop. It was initally 'finitely ascending' but that sound strange, so I flip the lattice to match that.
src/relay/pass/partial_eval.cc
Outdated
| auto ret = Meet(f, &progress); | ||
| return std::make_tuple(ret, progress); | ||
| } | ||
| /*! \brief return the new Fuel, and write true only iff progress is made. */ |
There was a problem hiding this comment.
| /*! \brief return the new Fuel, and write true only iff progress is made. */ | |
| /*! \brief return the new Fuel, and write true iff progress is made. */ |
There was a problem hiding this comment.
I am trying to communicate that if progress is already true, in no case it will be written false. I will rewordsmith.
| f_var = Var("f") | ||
| f = Function([x], If(op.equal(x, const(0)), const(0), x + f_var(x - const(1)))) | ||
| orig = run_infer_type(Let(f_var, f, f_var(const(10)))) | ||
| assert_alpha_equal(dcpe(orig), const(55)) |
There was a problem hiding this comment.
Why is this called test_triangle?
There was a problem hiding this comment.
triangle number
There was a problem hiding this comment.
can you change it to test_triangular_number?
MarisaKirisame
changed the title
src/relay/pass/partial_eval.cc
Outdated
|
|
||
| class FuelNode; | ||
| /*! \brief A meet-semilattice with finite descending chain. | ||
| * It mean that we can meet two element to get an element, |
src/relay/pass/partial_eval.cc
Outdated
| class FuelNode; | ||
| /*! \brief A meet-semilattice with finite descending chain. | ||
| * It mean that we can meet two element to get an element, | ||
| * and for every element, there is only finite amount of meet before getting back the same element. |
src/relay/pass/partial_eval.cc
Outdated
| * and for every element, there is only finite amount of meet before getting back the same element. | ||
| * | ||
| * Every time we recurse, we do a meet and require that progress must be made. | ||
| * This make sure we do not recurse infinitely in the Partial Evaluator. |
src/relay/pass/partial_eval.cc
Outdated
| Expr PostProcess(const Expr&); | ||
|
|
||
| /*! \brief The base container type of Relay values. */ | ||
| /*! \brief A StaticNode contain some static data that the Partial Evaluator can use. */ |
…checker for scalar. (apache#3703) * save lint some lint lint add charrnn save save save remove debug remove debug remove space refactor save rewrite dce * reset files * join -> meet * lint * address review comment * wordsmith
…checker for scalar. (apache#3703) * save lint some lint lint add charrnn save save save remove debug remove debug remove space refactor save rewrite dce * reset files * join -> meet * lint * address review comment * wordsmith
3 participants
This PR:
@jroesch @tmoreau89 @weberlo