marcoabreu commented on a change in pull request #15550: Infra to use tvm write
op kernels
URL: https://github.com/apache/incubator-mxnet/pull/15550#discussion_r304758981
##########
File path: Makefile
##########
@@ -589,6 +603,30 @@ $(DMLC_CORE)/libdmlc.a: DMLCCORE
DMLCCORE:
+ cd $(DMLC_CORE); $(MAKE) libdmlc.a USE_SSE=$(USE_SSE)
config=$(ROOTDIR)/$(config); cd $(ROOTDIR)
+TVM_USE_CUDA := OFF
Review comment:
No, what I mean is that the TVM related logic should be in a single place
which infers from our base variables USE_TVM, USE_CUDA and USE_CUDA_PATH. Any
variables that are now TVM specific should be constructed in a single block
that has "if USE_TVM" as condition. These variables don't leave the scope of
that block.
Right now, this introduces more and more TVM specific variables on a global
level and scattered across the makefile. Instead it would be good if we have a
single block to invoke TVM - this block will take care of converting all
existing variables to their TVM counterpart. This improves the modularity and
acts as a clear separation between our build system and TVM. It acts like a
"translator". The issue to me here is that we are mirroring existing variables
we already have at the global scope. Combining this fact with conditional
operations on top of these mirrored variables, makes it harder to understand
the process flow.
I hope that this makes it clearer
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
