WIP: Adding TRT options/task #435
Conversation
| // TODO (pranavm): Figure out a better way to reuse TRT translation options - | ||
| // maybe move to options providers? | ||
| struct TensorRTOptions | ||
| : public mlirtrt::compiler::OptionsProvider<TensorRTOptions> { | ||
| mlir::tensorrt::TensorRTTranslationOptions options; | ||
|
|
||
| void addToOptions(mlir::OptionsContext &context) { | ||
| options.addToOptions(context); | ||
| } | ||
| }; |
There was a problem hiding this comment.
I can move TensorRTTranslationOptions to make them an options provider if that makes sense to do.
| // TODO (pranavm): Check if this needs to be conditional - the TRT passes | ||
| // above are not. | ||
| #ifdef MLIR_TRT_TARGET_TENSORRT | ||
| mlirtrt::compiler::registerTensorRTToExecutableTask(); | ||
| #endif |
There was a problem hiding this comment.
I'm not sure what exactly needs to be guarded. In a lot of places we guard TRT things with MLIR_TRT_TARGET_TENSORRT but that doesn't seem to be the case for the TRT passes registered above.
There was a problem hiding this comment.
The TensorRT dialect and MLIR passes are always built. It doesn't depend on actually having TensorRT binaries or headers to build against (or at least, that was the idea, we haven't been enforcing it). The MLIR_TRT_TARGET_TENSORRT is basically a guard for anything that relies on having actual TRT headers or libraries to link against, for example the translation from MLIR to TensorRT, hence the TARGET_TENSORRT in the name.
| # TODO (pranavm): TensorRTToExecutable should probably be a separate library | ||
| TensorRTToExecutable.cpp |
There was a problem hiding this comment.
Given that there's an open TODO for TensorRTExtension.cpp I'm wondering if this is more complicated than it seems.
| TensorRTToExecutableOptions::TensorRTToExecutableOptions( | ||
| TaskExtensionRegistry extensions) { | ||
| // TODO (pranavm): Do we need to support extensions? | ||
| } |
There was a problem hiding this comment.
Not sure if we want to require extensions for all options types or if we need to handle both cases in the options registry. If it's the former, then I can just assert that the extensions are empty here (or maybe even just add support?). If it's the latter, we could have a setExtensions method so it becomes optional instead of having it part of the constructor.
| @@ -127,6 +127,18 @@ struct DeviceOptions : public OptionsProvider<DeviceOptions> { | |||
| llvm::Error finalizeImpl(); | |||
| }; | |||
|
|
|||
| struct EntrypointOptions : public OptionsProvider<EntrypointOptions> { | |||
There was a problem hiding this comment.
This could be called "common compilation options" or similar. Really 'entrypoint' shouldn't be required at all and should be eliminated from whatever passes require this (not required for this MR).
| #include "mlir-tensorrt/Compiler/OptionsProviders.h" | ||
| #include "mlir/Pass/PassManager.h" | ||
|
|
||
| using namespace mlirtrt::compiler; |
There was a problem hiding this comment.
We should never ever put using namespace ... into a header file
There was a problem hiding this comment.
Yeah, this is from a bunch of copy-pasting to get the scaffolding done. I'll clean everything up once I have something working.
pranavm-nvidia
force-pushed
the
trt-task
branch
from
137caf5 to
08f90f3
Compare
pranavm-nvidia
force-pushed
the
trt-task
branch
from
08f90f3 to
88d48b9
Compare
No description provided.