Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RELAY][TRANSFORM] Migrate buildmodule to transform #3251

Merged
merged 12 commits into from
Jun 3, 2019

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented May 28, 2019

This PR is the second item of #3202. We focus on the following items

  • Register optimization passes.
  • Remove RelayBuildConfig in BuildModule and replace them with PassContext.
  • Migrate Optimize to Sequential in BuildModule.
  • Add unit tests for Sequential using With scoping.

cc @tqchen @jroesch @MarisaKirisame @wweic @icemelon9

@MarisaKirisame
Copy link
Contributor

I dont think type inference should be a pass. It should be something we always call pre/post to doing anything.

@MarisaKirisame
Copy link
Contributor

also can you rebase?

include/tvm/relay/transform.h Outdated Show resolved Hide resolved
include/tvm/relay/pass.h Outdated Show resolved Hide resolved
include/tvm/relay/transform.h Outdated Show resolved Hide resolved
python/tvm/relay/transform.py Outdated Show resolved Hide resolved
python/tvm/relay/transform.py Outdated Show resolved Hide resolved
tests/cpp/relay_transform_sequential.cc Outdated Show resolved Hide resolved
tests/cpp/relay_transform_sequential.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented May 28, 2019

TypeInference could be a pass that can be required by some other passes

@zhiics
Copy link
Member Author

zhiics commented May 28, 2019

@MarisaKirisame Yes, I also think TypeInference should be a pass. Otherwise, we have to force to execute it explicitly.

@MarisaKirisame
Copy link
Contributor

I think it should be implicit, and all pass will call it pre/post the pass (this is register in passmanager.cc).
Type Inference do not change the runtime behavior of the program in anyway, so we should just always do them.
Even if some pass run on untyped code, I still dont think it is a good idea: running type inference help catch error early (and indeed multiple error had been caught by just running it).

@zhiics zhiics force-pushed the migrate_build_module branch from c674ed7 to 1d37f52 Compare May 28, 2019 21:11
@zhiics
Copy link
Member Author

zhiics commented May 28, 2019

@MarisaKirisame Yeah, I see your point, but shouldn't be better to add it to the required list and invoke it only when necessary?

@MarisaKirisame
Copy link
Contributor

@zhiics dont think of it as an analysis; think of it as a safety check.

src/relay/pass/pass_manager.cc Outdated Show resolved Hide resolved
@zhiics zhiics force-pushed the migrate_build_module branch from 9d55501 to 25fcc77 Compare May 28, 2019 22:51
@zhiics
Copy link
Member Author

zhiics commented May 28, 2019

I think @MarisaKirisame's point makes sense at some extend. But I still think it might be more appropriate to add TypeInference as a required pass and apply it when necessary. If we want to do safety check before every pass, we should probably encode type inference there.

In the future, we may need to do pass validation after optimization where type inference might be explicitly called any way. @MarisaKirisame Do you think this is okay? @tqchen @jroesch thoughts?

@tqchen
Copy link
Member

tqchen commented May 28, 2019

As a side note, to demonstrate the flexibility of the new transform API. I hope we can revive relay.build in python, which calls into the transform API without calling into C++. This is a slight divergence but given that the code is short enough, I believe a useful thing to do. Eventually, we can also have a DefaultOptPipeline that returns the default sequential pipeline we use for optimization.

tests/cpp/relay_transform_sequential.cc Outdated Show resolved Hide resolved
tests/cpp/relay_transform_sequential.cc Outdated Show resolved Hide resolved
@zhiics zhiics force-pushed the migrate_build_module branch 2 times, most recently from bcd2bec to 51f6013 Compare May 29, 2019 02:51
@jroesch
Copy link
Member

jroesch commented May 29, 2019

As a side note, to demonstrate the flexibility of the new transform API. I hope we can revive relay.build in python, which calls into the transform API without calling into C++. This is a slight divergence but given that the code is short enough, I believe a useful thing to do. Eventually, we can also have a DefaultOptPipeline that returns the default sequential pipeline we use for optimization.

Doesn't this actively work against the goal to have a pure C++ pipeline for the compiler?

@tqchen
Copy link
Member

tqchen commented May 29, 2019

What I mean is to have a python pipeline alone with the c++ one, given both of them are short enough, we want to make sure it is easy to customize compilation pipeline from python

@zhiics
Copy link
Member Author

zhiics commented May 29, 2019

yeah, we can probably have a separate discussion about adding the parallel optimization pipeline. Currently, we can finish the migration and clean up ir_pass.py and pass.h. How do you guys think?

include/tvm/relay/transform.h Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
@zhiics zhiics force-pushed the migrate_build_module branch from 51f6013 to 0e09241 Compare May 30, 2019 17:36
@zhiics zhiics force-pushed the migrate_build_module branch from 0e09241 to 83c2527 Compare May 30, 2019 17:39
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
@zhiics
Copy link
Member Author

zhiics commented May 31, 2019

@tqchen Now all passes are moved to Optimize and it is Module to Module.

@zhiics zhiics force-pushed the migrate_build_module branch from 3b64482 to a845b0f Compare May 31, 2019 06:32
Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

src/relay/backend/build_module.cc Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented May 31, 2019

I can merge this in as it is clearly an improvement over what we had before. However, I am still not happy with the Optimize. Here is a mock example of an "ideal code" of optimize:

# mostly device invariant optimizations
opt = transform.Sequential([
    transform.FoldConstant(),
    transform.SimplifyInference(),
    transform.EnableIf(lambda ctx: len(ctx.targets) == 1, transform.AlterLayout()),
 ])

# target specific optimizations
lower = transform.Sequential([
       transform.DeviceAnnotation(), # only enables for multiple target
       transform.FuseOps()
    ])

with transform.PassContext(target=target_device_map):
     mod = optimzie(mod)
     mod = lower(mod)

This is only a quick alternative that pops out from my head. @zhiics feel free to the critic. But we should really move to minimize the code of this lowering process.

Also cc @merrymercy @vinx13 @yzhliu @jroesch for more thoughts, perhaps we could use another RFC if necessary :)

@tqchen tqchen changed the title [relay][transform] Migrate buildmodule to transform [RELAY][TRANSFORM] Migrate buildmodule to transform May 31, 2019
src/relay/pass/pass_manager.cc Outdated Show resolved Hide resolved
src/relay/pass/pass_manager.cc Outdated Show resolved Hide resolved
Copy link
Member

@tqchen tqchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some further nits

python/tvm/relay/transform.py Outdated Show resolved Hide resolved
@zhiics
Copy link
Member Author

zhiics commented May 31, 2019

I can merge this in as it is clearly an improvement over what we had before. However, I am still not happy with the Optimize. Here is a mock example of an "ideal code" of optimize:
This is only a quick alternative that pops out from my head. @zhiics feel free to the critic. But we should really move to minimize the code of this lowering process.

Sorry. I totally missed this. I totally agree with you that it makes sense to separate target dependent and independent opts. Actually, I have been thinking of this as well. We can probably have another RFC for it. But let us keep this code change minimum for now.

@tqchen tqchen merged commit bb48a45 into apache:master Jun 3, 2019
@tqchen
Copy link
Member

tqchen commented Jun 3, 2019

Thanks @wweic @zhiics

@zhiics zhiics deleted the migrate_build_module branch June 4, 2019 18:07
wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants