[CORE][Relay] Swap and remove compile_engine with te_compiler followup of #8775 #9282

mikepapadim · 2021-10-14T11:10:46Z

This is a follow-up work for #8775 to address the pending comments to remove the compile_engine.

As this is an intermediate step towards completely moving away from the engine, we exposed the following legacy functions from the compile engine to ensure backwards compatibility atm:

_TECompilerJIT
_TECompilerLower
_make_LoweredOutput
_make_CCacheKey

Now, compile_engine.py has been replaced with te_compiler.py and deprecated funcs, such as LowerShapeFuncs kept private within the te_compiler.cc.

@jroesch @mbs-octoml @electriclilies @comaniac @junrushao1994

…ile-engine-for-real

electriclilies · 2021-10-15T19:09:46Z

python/tvm/relay/backend/te_compiler.py

+        try:
+            mod_name = mangle_module_name(mod_name)
+            key = _get_cache_key(source_func, target)
+            print(key)


A stray print!

electriclilies · 2021-10-15T19:10:09Z

python/tvm/relay/backend/te_compiler.py

-            ret.append(dim)
-    return ret
-
-


What is this replaced with?

Oops, now I see this was just moved. Nevermind

tests/python/contrib/test_vitis_ai/infrastructure.py

electriclilies · 2021-10-15T19:23:24Z

python/tvm/relay/backend/te_compiler.py

+        """
+        # pylint: disable=broad-except, import-outside-toplevel
+        try:
+            mod_name = mangle_module_name(mod_name)


Why do name mangling here instead of in TECompilerLower?

This is directly taken from the original compile_engine. I agree we can move it to C++ through te_compiler, but I see a few other usages of it within the codebase.

electriclilies · 2021-10-15T19:31:08Z

Overall, LGTM, I just have a few nitpicks. I took a quick look at what is failing and it looks like there are some issues with the registration, but once those are resolved this looks like it is good to go in! Thanks!

mikepapadim · 2021-10-18T08:32:55Z

PTAL @mbs-octoml @jroesch @electriclilies

Mousius

Mostly this looks good, just found a few bits to tidy

python/tvm/relay/backend/te_compiler.py

tests/python/contrib/test_vitis_ai/infrastructure.py

Mousius · 2021-10-18T12:00:06Z

tests/python/relay/aot/aot_test_utils.py

@@ -721,7 +723,7 @@ def compile_and_run(

 def generate_ref_data(mod, input_data, params=None, target="llvm"):
    """Generate reference data through executing the relay module"""
-    compile_engine.get().clear()
+    te_compiler.get().clear()


Do we even need this anymore for AOT?

Indeed, i think is not required anymore. It was transilient from compile engine.

mikepapadim · 2021-10-18T12:55:23Z

Mostly this looks good, just found a few bits to tidy

Thanks, just pushed the fixes.

mbs-octoml

LGTM thank you Michalis.

mbs-octoml · 2021-10-18T15:45:13Z

python/tvm/autotvm/graph_tuner/utils/traverse_graph.py

@@ -142,7 +142,7 @@ def _traverse_expr(node):
                    params.append(free_var)
                call = relay.Call(node.op, params, node.attrs)
                mod = tvm.IRModule.from_expr(relay.Function(params, call))
-                relay.backend.compile_engine.get().clear()
+                relay.backend.te_compiler.get().clear()


I didn't realize there were so many uses of the dreaded global. Could you please file a JIRA issue to remove them? I suspect simply creating locally would be fine and this has been done just for 'efficiency'. If not we have bigger problems.

Just filled a ticket on it. I did locally for a few test with no issues. So, after this on is merged. I can sent a followup PR unifying all usages.

comaniac

LGTM.
@icemelon please also take a look, as this may affect our use cases.

electriclilies

LGTM, thanks so much!

jroesch · 2021-10-19T06:54:59Z

I will wait some time for Haichen before merging.

mikepapadim · 2021-10-19T07:46:58Z

I will wait some time for Haichen before merging.

Thanks! We can do some followups as @mbs-octoml suggested.

mikepapadim · 2021-10-21T15:40:58Z

@jroesch should we merge this one?

…p of apache#8775 (apache#9282) * Remove compile_engine.h for real * Fix format * RM compile_engine.cc * Swap compile engine with TECompiler * Cleanup on compile engine py leftovers * [WIP] Exposing legacy compile engine capabilities through TE Compiler * Swap usages for depreciated compile engine with TE compiler * Track and replace usages of compile engine refactor them to TE compiler * [Docs] Log helper mod * Remove depreciated function for lookup compile engine cachce * Fix typos * Debug misc cleanups * Register global pass for using te compiler for auto scheduler * Fix tests using the legacy compile engine * Fix broken autotuner tests and minor cleanups * Swap compile engine with te_compiler in rst config * PR nits * Fix failed test Co-authored-by: Jared Roesch <[email protected]>

jroesch and others added 14 commits August 17, 2021 16:52

Remove compile_engine.h for real

7d9d9c9

Fix format

79d42e5

Fix merge conflicts

65a2943

RM compile_engine.cc

efa80c4

Swap compile engine with TECompiler

4e5840e

Cleanup on compile engine py leftovers

0065463

[WIP] Exposing legacy compile engine capabilities through TE Compiler

b1f817a

Swap usages for depreciated compile engine with TE compiler

e4bd0d8

Track and replace usages of compile engine refactor them to TE compiler

d9b54eb

[Docs] Log helper mod

13260db

Remove depreciated function for lookup compile engine cachce

820689e

Fix typos

c85da07

Merge branch 'main' of https://github.com/apache/tvm into remove-comp…

1ccd14c

…ile-engine-for-real

Debug misc cleanups

b8b9e82

mikepapadim requested review from anijain2305, areusch, comaniac, Huyuwei, jcf94, jroesch, junrushao, jwfromm, kazum, kevinthesun, Laurawly, liangfu, MarisaKirisame, masahi, mbrookhart and merrymercy as code owners October 14, 2021 11:10

Register global pass for using te compiler for auto scheduler

62740c5

mikepapadim requested a review from Hzfengsy as a code owner October 15, 2021 09:32

Fix tests using the legacy compile engine

cd021b7

mikepapadim force-pushed the rm-ce-michalis branch from 669485a to cd021b7 Compare October 15, 2021 09:37

electriclilies reviewed Oct 15, 2021

View reviewed changes

tests/python/contrib/test_vitis_ai/infrastructure.py Outdated Show resolved Hide resolved

electriclilies reviewed Oct 15, 2021

View reviewed changes

Fix broken autotuner tests and minor cleanups

1df8132

mikepapadim mentioned this pull request Oct 18, 2021

Remove compile_engine.h for real this time. #8775

Closed

Mousius requested changes Oct 18, 2021

View reviewed changes

Swap compile engine with te_compiler in rst config

5100263

mikepapadim force-pushed the rm-ce-michalis branch from 1baaa7d to 5100263 Compare October 18, 2021 12:27

mbs-octoml approved these changes Oct 18, 2021

View reviewed changes

mikepapadim added 2 commits October 18, 2021 19:33

PR nits

1a53713

Fix failed test

3f08424

mikepapadim force-pushed the rm-ce-michalis branch from 3ea3f4a to 3f08424 Compare October 18, 2021 16:34

comaniac reviewed Oct 18, 2021

View reviewed changes

electriclilies approved these changes Oct 18, 2021

View reviewed changes

jroesch approved these changes Oct 19, 2021

View reviewed changes

masahi merged commit 6ef1c2a into apache:main Oct 22, 2021

Mousius mentioned this pull request Dec 14, 2022

[Relay][TIR] Add utility to lower Relay func to TIR prim func #13606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE][Relay] Swap and remove compile_engine with te_compiler followup of #8775 #9282

[CORE][Relay] Swap and remove compile_engine with te_compiler followup of #8775 #9282

mikepapadim commented Oct 14, 2021

electriclilies Oct 15, 2021

mikepapadim Oct 18, 2021

electriclilies Oct 15, 2021

electriclilies Oct 15, 2021

electriclilies Oct 15, 2021

mikepapadim Oct 18, 2021

electriclilies commented Oct 15, 2021

mikepapadim commented Oct 18, 2021

Mousius left a comment

Mousius Oct 18, 2021

mikepapadim Oct 18, 2021

mikepapadim commented Oct 18, 2021

mbs-octoml left a comment

mbs-octoml Oct 18, 2021

mikepapadim Oct 18, 2021

comaniac left a comment

electriclilies left a comment

jroesch commented Oct 19, 2021

mikepapadim commented Oct 19, 2021

mikepapadim commented Oct 21, 2021

[CORE][Relay] Swap and remove compile_engine with te_compiler followup of #8775 #9282

[CORE][Relay] Swap and remove compile_engine with te_compiler followup of #8775 #9282

Conversation

mikepapadim commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electriclilies commented Oct 15, 2021

mikepapadim commented Oct 18, 2021

Mousius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikepapadim commented Oct 18, 2021

mbs-octoml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comaniac left a comment

Choose a reason for hiding this comment

electriclilies left a comment

Choose a reason for hiding this comment

jroesch commented Oct 19, 2021

mikepapadim commented Oct 19, 2021

mikepapadim commented Oct 21, 2021