-
Notifications
You must be signed in to change notification settings - Fork 323
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
Import dim_param for model inputs and outputs #2616
Conversation
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
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, I had wanted something like this for a long time, it's great that it is part of the ONNX protocol so that we don't have to reinvent the wheel. With our efficient dimAnalysis, this should really help out disambiguating dynamic dimensions that are really identical.
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.
really nice PR, I had always hoped that your dynamic dimensions framework would apply to onnx's dim_params
@@ -324,7 +334,8 @@ class FrontendGenImpl { | |||
onnx::TypeProto elem_type = input_seq_type.elem_type(); | |||
assert(elem_type.value_case() == onnx::TypeProto::kTensorType && | |||
"expect tensor inside sequence type"); | |||
Type mlir_elem_type = ImportTensorType(elem_type); | |||
std::string s; |
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.
a little bit easier to read if you rename s
to dimParam
or consider adding convenience methods ImportTensorType
and ImportType
without the dimParam parameter, or make dimParam a pointer that defaults to nullptr
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.
Done. Changed to use optional arguments.
@@ -287,7 +288,8 @@ class FrontendGenImpl { | |||
* Import an onnx tensor type by determining and returning its type | |||
* @param type_proto onnx tensor TypeProto. |
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.
please add a @param description of dimParam
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.
Done. Thanks!
SmallVector<llvm::StringRef> inputDimParamsRefs, outputDimParamsRefs; | ||
for (uint64_t i = 0; i < inputDimParams.size(); ++i) | ||
inputDimParamsRefs.emplace_back(llvm::StringRef(inputDimParams[i])); | ||
for (uint64_t i = 0; i < outputDimParams.size(); ++i) | ||
outputDimParamsRefs.emplace_back(llvm::StringRef(outputDimParams[i])); | ||
op->setAttr("input_names", builder_.getStrArrayAttr(inputNames)); | ||
op->setAttr("output_names", builder_.getStrArrayAttr(outputNames)); | ||
op->setAttr( | ||
"input_dim_params", builder_.getStrArrayAttr(inputDimParamsRefs)); | ||
op->setAttr( | ||
"output_dim_params", builder_.getStrArrayAttr(outputDimParamsRefs)); |
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 think it would be nicer if we made all of these argument and result attributes
https://github.com/sorenlassen/llvm-project/blob/main/mlir/include/mlir/Dialect/Func/IR/FuncOps.td#L241-L245
func.func @main_graph(
%arg0: tensor<?x?xf32> {onnx.name = "X", onnx.dims = "0:batch_size,1:sequence_len"},
%arg1: tensor<1x?xi64> {onnx.name = "Y", onnx.dims = "1:sequence_len"}
) -> (
tensor<?x?xf32> {onnx.name = "Z", onnx.dims = "0:batch_size,1:sequence_len"}
)
and then you can omit the onnx.dims attribute in the cases where it's empty
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.
Great suggestion! I moved to use argument/result attributes instead of function attributes. Thanks!
model_def = helper.make_model(graph_def, producer_name="onnx-mlir") | ||
|
||
onnx.checker.check_model(model_def) | ||
print(MessageToJson(model_def)) |
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.
if you like the more concise textual representation of the .onnxtext lit tests you can change the last line to print(onnx.printer.to_text(model_def))
- see utils/onnx2text.py
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.
It's quite simple so let me keep the current one.
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Signed-off-by: Tung D. Le <[email protected]>
Jenkins Linux ppc64le Build #12382 [push] Import dim_param for mod... started at 05:34 |
Jenkins Linux amd64 Build #13362 [push] Import dim_param for mod... started at 04:26 |
Jenkins Linux s390x Build #13388 [push] Import dim_param for mod... started at 05:26 |
Jenkins Linux amd64 Build #13362 [push] Import dim_param for mod... failed after 1 hr 13 min |
Jenkins Linux s390x Build #13388 [push] Import dim_param for mod... passed after 1 hr 27 min |
Jenkins Linux ppc64le Build #12382 [push] Import dim_param for mod... passed after 1 hr 47 min |
* detect LayerNorm in presence of reciprocal and div of 1 (onnx#2609) Signed-off-by: Alexandre Eichenberger <[email protected]> * [NNPA] Use F16 as element type for zTensor (onnx#2611) * Use f16 as element type for zTensor Signed-off-by: Tung D. Le <[email protected]> --------- Signed-off-by: Tung D. Le <[email protected]> * Layernorm: convert instance norm and group norm to layer norm. (onnx#2595) Signed-off-by: Alexandre Eichenberger <[email protected]> Co-authored-by: Tung D. Le <[email protected]> * Parse and set --mcpu in onnx-mlir-opt command (onnx#2614) Signed-off-by: Tung D. Le <[email protected]> * Import dim_param for model inputs and outputs (onnx#2616) * Import dim_param for model inputs and outputs * use argument attributes Signed-off-by: Tung D. Le <[email protected]> --------- Signed-off-by: Tung D. Le <[email protected]> Co-authored-by: Alexandre Eichenberger <[email protected]> * [DialectBuilder] add builder funcrions for ONNXSumOp and ONNXConvOp (onnx#2572) The DialectBuilder class seems to be missing the function create the ONNXSumOp and ONNXConOp nodes and check their shape. This patch adds the necessary functions. Signed-off-by: Ashay Rane <[email protected]> Signed-off-by: Alexandre Eichenberger <[email protected]> Co-authored-by: Alexandre Eichenberger <[email protected]> * [StableHLO] Lowers PadOp (constant mode) & GatherElements Op to StableHLO (onnx#2602) * [Stablehlo] Pad constant mode & GatherElements to Stablehlo Signed-off-by: chongsong.chen <[email protected]> Signed-off-by: Yan Xu <[email protected]> Co-authored-by: chongsong.chen <[email protected]> Co-authored-by: Alexandre Eichenberger <[email protected]> * [build] Add cmake option to enable/disable Java components build (onnx#2613) * Add ONNX_MLIR_ENABLE_JAVA cmake option (default TRUE) Signed-off-by: Boyana Norris <[email protected]> Co-authored-by: Alexandre Eichenberger <[email protected]> Co-authored-by: Alexandre Eichenberger <[email protected]> Co-authored-by: Tung D. Le <[email protected]> Co-authored-by: Ashay Rane <[email protected]> Co-authored-by: Yan Xu <[email protected]> Co-authored-by: chongsong.chen <[email protected]> Co-authored-by: Boyana Norris <[email protected]>
* 'main' of github.ibm.com:zosdev/onnx-mlir: Use dim_params in dynamic dimension analysis (onnx#2620) Update rapidcheck to include the fix for missing <cstdint> include (onnx#2623) Initial changes for llvm uplift (onnx#2568) [build] Add cmake option to enable/disable Java components build (onnx#2613) [StableHLO] Lowers PadOp (constant mode) & GatherElements Op to StableHLO (onnx#2602) [DialectBuilder] add builder funcrions for ONNXSumOp and ONNXConvOp (onnx#2572) Import dim_param for model inputs and outputs (onnx#2616) Parse and set --mcpu in onnx-mlir-opt command (onnx#2614) Layernorm: convert instance norm and group norm to layer norm. (onnx#2595) [NNPA] Use F16 as element type for zTensor (onnx#2611) detect LayerNorm in presence of reciprocal and div of 1 (onnx#2609) # Conflicts: # test/mlir/conversion/onnx_to_krnl/NN/Normalization_O3_SIMD_canonicalize.mlir
In ONNX specification, a dimension can be
dim_value
(static) ordim_param
(a named dynamic dimension).If two dynamic dimensions has the same
dim_param
, they must have the same value at runtime.This patch will import
dim_param
of the inputs and outputs of a model, and embed suchdim_param
in argument and result attributes. This patch also movesinput_names
andoutput_names
into argument and result attributesonnx.name
. for exampleonnx.dim_params
is a list of"dim_index:dim_param"
In a practical model, e.g. t5-decoder with KV cache,
dim_param
is like the following, and we know that all the first dimensions (batch_size) must be the same.These
dim_param
information will be used later by the dynamic dimension analysis to make the analysis stronger. Also we can check if users' inputs to the model are consistent or not.