-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[MetaSchedule] Support RewriteLayout postproc on AllocateConst #12991
Conversation
bd0ea26
to
a3eba9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please fix the CI and let's get it in
a3eba9b
to
1134c5a
Compare
26df009
to
a25a3d3
Compare
There are still some issues with Hexagon tests, debugging. |
) | ||
return bool(result & 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes in this file are probably necessary after the MS API change PR - without them I got a compile error and segfault using compile_relay
. cc @junrushao
bf6f2da
to
074f48a
Compare
if executor is None: | ||
executor = relay.backend.Executor("graph") | ||
|
||
if mod.get_attr("executor") is None: | ||
mod = mod.with_attr("executor", executor) | ||
else: | ||
executor = mod.get_attr("executor") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick question: why do we need the logic on line 97-98? i.e. is non-nullable executor
useful in subsequent processing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When None executor
is passed to relay.build(..., executor=executor)
, we get segfault. This is certainly due to some sloppy coding somewhere, but I haven't looked into it.
Perhaps the default value of executor
should be Executor("graph")
rather than None, as in relay.build
https://github.com/apache/tvm/blob/main/python/tvm/relay/build_module.py#L281
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! This is certainly annoying that it’s engineered this way…Thanks for your explanation!
We probably don’t need to over-engineer it for now. Let’s keep it as it is given vm is not supported.
…e#12991) * [Metaschedule] Support AllocateConst in RewriteLayout * fix * convert constant back to original before trace apply * improved CreatePrimFunc change * use IndexMap to transform constant in RemoveLayoutRewriteBlock * wip * wip * add comments * add test * enable RewriteLayout on Hexagon * lint * improve RewriteLayout postproc * fix compiler warning * fixed CreatePrimFunc test * black * Fix after MS API change * fixed auto tensorize test after MS API PR * add missing Clone method to MultilevelTilingWideVector * fixed hexagon dense MS test * fixed the rest of Hexagon MS tests
RewriteLayout
assumes that the "layout-free" weight to be rewritten is passed as a parameter to prim func. This doesn't hold whenlink-params = True
is used, since the constant weight is directly embedded in a primfunc asAllocateConst
node.Supporting RewriteLayout on AllocateConst required some nasty hacks to workaround various issues. Detailed comments are provided to explain the problem and my solution.
This lets us enable
RewriteLayout
for Hexagon.