[Driver] Remove duplicate PreProcessModuleForBuild #10530
Conversation
Lunderberg
requested review from
comaniac,
icemelon,
jroesch,
junrushao,
jwfromm,
leandron,
tqchen,
yzhliu and
zhiics
as code owners
`PreProcessModuleForBuild` was nearly identical to the `build()` function in the same file.
Lunderberg
force-pushed
the
driver_api_remove_duplication
branch
from
e3d73a5 to
17b8dac
Compare
|
Rebased onto main following #10535. |
python/tvm/driver/build_module.py
Outdated
| annotated_mods, target_host = Target.check_and_update_host_consist(annotated_mods, target_host) | ||
|
|
||
| rt_mod_host = _driver_ffi.preprocess_module(annotated_mods, target_host) | ||
| rt_mod_host = _driver_ffi.build(annotated_mods, target_host) |
There was a problem hiding this comment.
There are some difference related "overrode the target host" logic between 'preprocess_module' and 'build', is this change may fix any potential corner case or cause any new corner case issue?
There was a problem hiding this comment.
From my understanding, I think this would fix accidental recursion, if a user has implemented a TIRToRuntime hook in Python, and that TIRToRuntime hook internally calls the build_module.build function. For a C++ implementation of TIRToRuntime, a user would call tvm::build directly, and get the recursion check, but that doesn't exist in PreProcessModuleForBuild. That said, I haven't tested it to verify whether this failure mode could be triggered.
I did some delving into the git commit history prior when trying to understand if there was an intentional difference between the functions, and I think this difference is due to accidental drift. From the commit history, both PreProcessModuleForBuild (PR #9103, later renamed in PR #9297) and the introduction of the TIRToRuntime hook (PR #9190) were occurring at roughly the same time, with #9190 being submitted and merged while #9103 was in review.
| return {host_mod, device_mod}; | ||
| } | ||
|
|
||
| runtime::Module PreProcessModuleForBuild(const Map<Target, IRModule>& inputs_arg, |
There was a problem hiding this comment.
I would almost prefer we use this name or maybe come up with an even better one from build especially in internal APIs since it is not very clear what build means. We have to preserve for historical API compatibility for now but seems better to not keep in C++ if we can.
There was a problem hiding this comment.
Good point, since build is used pretty frequently elsewhere, and could be confusing to users. Since most places use "build" to refer to generating something that is executable, I thought it would be appropriate here. But I think I agree, since in most places "build" has an entire sequence of steps, and here there's only the one.
I'm not a big fan of the name PreProcessModuleForBuild, because I read "PreProcess" as implying that this is a pretty early step in the build pipeline, rather than being nearly the last step.
The best name coming to mind is CodeGenerationForLowLevelTIR or GenerateCodeForLowLevelTIR, but waiting for the caffeine to hit may result in better names.
There was a problem hiding this comment.
Looking at it with fresh post-weekend eyes, it looks like this function is already part of the external API, as one of the overloads for tvm::build.
As a proposal for naming conventions, I've made an internal tvm::TIRToRuntime, with the name chosen to mimic the existing target-specific codegen hook. All internal usage calls tvm::TIRToRuntime directly, while the tvm::build overload is maintained for the external API.
huajsj
left a comment
huajsj
left a comment
There was a problem hiding this comment.
LGTM, thanks @Lunderberg.
* [Driver] Remove duplicate PreProcessModuleForBuild `PreProcessModuleForBuild` was nearly identical to the `build()` function in the same file. * Name change, `tvm::TIRToRuntime`.
3 participants
PreProcessModuleForBuildwas nearly identical to thebuild()function in the same file. The only difference between the two was thatPreProcessModuleForBuildwas missing a recursion check when usingTIRToRuntimehooks, which appears to have been accidental drift from two simultaneous PRs.