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

[Unity] Include LegalizeOps in the default relax.build lowering flow #15864

Merged

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, relax.transform.LegalizeOps needed to be called prior to relax.build. This commit adds LegalizeOps to the lowering flow, to simplify the calling steps for an end-user. If the IRModule contains no legalizable functions, a second legalization pass has no effect.

Some test cases relied on this behavior as an implicit assertion that operator fusion patterns applied. That is, by omitting LegalizeOps, a successful compilation relax.build would only occur if all legalizable operators have already been removed, and so an incorrect fusion pattern would result in a failure to build the module. While these tests would be better expressed by comparing against an expected fused pattern, updating the tests is outside the scope of this PR. To allow these tests to keep their implicit assertions, a "relax.transform.apply_legalize_ops" config can be used to disable the LegalizeOps pass.

@Lunderberg Lunderberg force-pushed the unity_default_legalize_ops_when_building branch from 16a7df0 to b821d53 Compare October 4, 2023 14:13
@junrushao
Copy link
Member

I'm not super sure if it's desirable. Usually after operator lowering, we will apply a series of transformations to reach the end state of the desirable TIR, which we could abstract with relax.pipeline, while the default relax.build is kept as minimum as possible to be compatible with all potential relax pipelines. One direction I would imagine is to introduce more pipelines so that the build process could be more streamlined

@quic-sanirudh
Copy link
Contributor

quic-sanirudh commented Oct 5, 2023

I'm not super sure if it's desirable. Usually after operator lowering, we will apply a series of transformations to reach the end state of the desirable TIR, which we could abstract with relax.pipeline, while the default relax.build is kept as minimum as possible to be compatible with all potential relax pipelines. One direction I would imagine is to introduce more pipelines so that the build process could be more streamlined

I think having LegalizeOps as part of relax.build in some format is useful as we've had multiple queries in our discuss forum about unsupported operators, which just turned out to be because users were not aware of having to call LegalizeOps explicitly before relax.build.

I also agree with the idea of keeping relax.build as minimal as possible and using relax.pipeline for the rest, so would it be better if update relax.build to take a relax pipeline as an argument and run the passes in the pipeline as part of the build process. That way, we can add a small pipeline with just LegalizeOps and make that the default pipeline taken by relax.build, and others who want customization can pass their own (probably more complex) pipelines.

@Lunderberg
Copy link
Contributor Author

@junrushao Absolutely agreed, there's a wide variety of hand-selected transformations that optimized models require, many of which must be performed after LegalizeOps. That is essential functionality, and this PR isn't intended to replace those at all.

Workflow 1: A user writes a model, applies writes a sequence of transformations that have been hand-selected to optimize that specific model into the desired form, then passes the result into relax.build. All operations have already been legalized, so applying LegalizeOps inside relax.build has no effect.

Workflow 2: A user writes a model, then passes the model directly into relax.build. The model still contains operators that require legalization, so LegalizeOps has an effect.

The goal of this PR is to enable Workflow 2 as a simpler way to get started, either for a new user or when running a new model, without impacting the optimizations performed in Workflow 1.

@Lunderberg
Copy link
Contributor Author

Philosophically, I like the distinction @quic-sanirudh is drawing between the minimal relax.build and the more expansive options in relax.pipeline. I'd see relax.build as a tool which converts from any well-formed relax module to an executable, performing any legalizations that are required, but which avoids performing any optimizations as those are outside of its scope.

@Lunderberg Lunderberg force-pushed the unity_default_legalize_ops_when_building branch from 9c0fa78 to c9762bc Compare October 24, 2023 19:47
@Lunderberg
Copy link
Contributor Author

I rebased onto current unity, to make sure that no CI breakages have emerged since then. Any objections to merging if the CI passes?

@quic-sanirudh
Copy link
Contributor

I agree with the workflow 2 you've mentioned here, and since LegalizeOps probably has no effect on workflow 1, I'm fine with merging this.

@junrushao
Copy link
Member

I don’t object either. Happy to merge it in

@Lunderberg
Copy link
Contributor Author

Sounds good, and thank you!

I'm rebasing the commit on top of unity head, as the current CI failures are due to a breakage on unity head, and are resolved with #15941. (See this comment for details.)

Prior to this commit, `relax.transform.LegalizeOps` needed to be
called prior to `relax.build`.  This commit adds `LegalizeOps` to the
lowering flow, to simplify the calling steps for an end-user.  If the
`IRModule` contains no legalizable functions, a second legalization
pass has no effect.

Some test cases relied on this behavior as an implicit assertion that
operator fusion patterns applied.  That is, by omitting `LegalizeOps`,
a successful compilation `relax.build` would only occur if all
legalizable operators have already been removed, and so an incorrect
fusion pattern would result in a failure to build the module.  While
these tests would be better expressed by comparing against an expected
fused pattern, updating the tests is outside the scope of this PR.  To
allow these tests to keep their implicit assertions, a
`"relax.transform.apply_legalize_ops"` config can be used to disable
the `LegalizeOps` pass.
@Lunderberg Lunderberg force-pushed the unity_default_legalize_ops_when_building branch from c9762bc to d78b932 Compare October 25, 2023 14:16
@Lunderberg Lunderberg merged commit ebbe38f into apache:unity Oct 26, 2023
14 checks passed
@Lunderberg Lunderberg deleted the unity_default_legalize_ops_when_building branch October 26, 2023 15:16
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Jun 18, 2024
This is a follow-up to apache#15864, which
added `LegalizeOps` to the default Relax build pipeline.  Since
legalization may produce additional TIR PrimFuncs that require
scheduling, the output of `LegalizeOps` typically must also be passed
through `tir.transform.DefaultGPUSchedule()`.  This PR adds
`DefaultGPUSchedule()` to the relax build pipeline to handle these
cases.

Scheduled PrimFunc have the `"tir.is_scheduled"` attribute set to
true, and are ignored by `DefaultGPUSchedule()`.  In addition, the
`DefaultGPUSchedule` transform has no effect on non-GPU targets.
Therefore, this change should only impact `tvm.relax.build` calls that
previously resulted in an error due to unscheduled GPU functions, and
should not have any impact on existing calls.
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Sep 11, 2024
This is a follow-up to apache#15864, which
added `LegalizeOps` to the default Relax build pipeline.  Since
legalization may produce additional TIR PrimFuncs that require
scheduling, the output of `LegalizeOps` typically must also be passed
through `tir.transform.DefaultGPUSchedule()`.  This PR adds
`DefaultGPUSchedule()` to the relax build pipeline to handle these
cases.

Scheduled PrimFunc have the `"tir.is_scheduled"` attribute set to
true, and are ignored by `DefaultGPUSchedule()`.  In addition, the
`DefaultGPUSchedule` transform has no effect on non-GPU targets.
Therefore, this change should only impact `tvm.relax.build` calls that
previously resulted in an error due to unscheduled GPU functions, and
should not have any impact on existing calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants