Node setDatatype is too strict

Context

Current setDatatype is too strict and override output datatype where it should not.

An example is the Shape Operator that should always return a INT64 Tensor by design.

I propose to introduce a new method forwardDataType that for a given node set the datatype of the output using the datatype of its input.

A current use case is the shuffle layer: https://gitlab.eclipse.org/-/project/5139/uploads/6ad9fc3fc64740b31b696a3f20308553/shuffle_layer.onnx

Ideally all the dim computation should be done using INT64

Implementation idea

This would be done by adding forwardDataType in OperatorTensor. By default, this function check that every input has the same datatype and set this datatype to the outputs.

This would be override by specific operator such as:

• Reshape: Input 1 need to be INT64, output is the same as input 0
• Shape: Whatever the input, the output is always INT64.

In order to maintain current API, GraphView::setDatatype would call GraphView::forwardDataType with a deprecation warning.

GraphView::forwardDataType would have two signatures:

• bool GraphView::forwardDataType(dtype): Return true if successfull. the same dtype is forwarded to all inputs.
• bool GraphView::forwardDataType(std::vector<dtype>): Return true if successfull. Specify different dtype for each graph inputs.

added Discussion 💬 Enhancement ⭐ labels

@olivierbichler @pineapple What are your thoughts?

I know this is a problem @gregkub is currently facing. But that @axelfarr and @mick94 also faced in the past when trying to set the graph to INT8.

Indeed it is a real issue when importing complex graphs. The current GraphView::setDataType has made its time.

I set this issue priority to "high" because it is blocking for many people.

Yes I had issues with this. Recently using setDatatype to yolov8 sets all the dtypes of all operators to float32, even when it should not. Meaning that expand has its 2nd input set to float32 when it only accepts int64. Hence I don't find corresponding implems for expand.

However I would like to point a potential issue we will encounter in the forwardDatatype : in yoloV8 there are sometimes multiplications of float and int, what will be the output dtype in this case ?

I would propose to follow the standard of C++ for this specific case but what about specific operators like resize ? I think that the forwardDataType function might need to be specific for each operator.

Also since these functions would define the in/out dtype, they ultimately should be used to create the registrar outputs as well in order to be sure that registrar entries are in synch with the function.

However I would like to point a potential issue we will encounter in the forwardDatatype : in yoloV8 there are sometimes multiplications of float and int, what will be the output dtype in this case ?

There is no such operation in your model. It is suppose to be both INT64.

I would propose to follow the standard of C++ for this specific case but what about specific operators like resize ? I think that the forwardDataType function might need to be specific for each operator.

What standard?

Also since these functions would define the in/out dtype, they ultimately should be used to create the registrar outputs as well in order to be sure that registrar entries are in synch with the function.

I think you talk about ImplSpec. But not sure what the link would be? ImplSpec link to a implementation given in/out dtype while this function provide out dtype given inputs.

Int64 mul

Ok my bad I just double checked its the setDataType call that messed up the operator

Standard

I'm tallking abt the implicit casts in an operation like the following

auto a = 1 * 1.2 

a 's inferred type is double. But this is specific for the arithmetic operators.

ImplSpec

Yes, I'm talking about them the implSpecs.

The REGISTRAR macro could be rewritten as :

REGISTRAR(FCImpl_cpu,
    {ImplSpec::IOSpec{DataType::Int32},
     ImplSpec::IOSpec{FC_Op::forwardDataType(Data::Int32)}}, <---- HERE
    {ProdConso::defaultModel,
     Aidge::FCImpl_cpu_forward_kernel<int32_t, int32_t, int32_t, int32_t>, Aidge::FCImpl_cpu_backward_kernel<int32_t, int32_t, int32_t, int32_t>});

This would ensure that Impl Specs are in synch with datatypes generated by the forwardDims function.

However this would reduce code readability as the output type would not be explicitely be written down

I am heavily against auto cast in the context of operators and I think graph should be valid data type wise. Potentially adding a cast operator in the graph.

Regarding ImplSpec, it can be done here but may be annoying and not add readability in the long run, since it will force to create a lot of functions (or lambdas) that will not be readable in this code block.

Especially you added it as an argument of the registration mecanism but the function should be returned by the registrar.

So it would be something like this (trying to make things readable with lambda):

REGISTRAR(FCImpl_cpu,
    {ImplSpec::IOSpec{DataType::Int32},
    {ProdConso::defaultModel,
     [](std::vector<DataType>){return x[0];},    // <-- HERE
     Aidge::FCImpl_cpu_forward_kernel<int32_t, int32_t, int32_t, int32_t>, Aidge::FCImpl_cpu_backward_kernel<int32_t, int32_t, int32_t, int32_t>});

IMO forwarding data types is implementation indepedant so I viewed it as a method of the Operator.

added PriorityHigh label

mentioned in merge request !363

assigned to @cmoineau

added StatusWork in Progress label

mentioned in merge request aidge_onnx!115 (merged)

@olivierbichler

Example where we need to "forward" datatype:

The Relu take an input int8 after adding the cast from the PTQ but its output is still set to int64!

An other need to forward tensor information is related to dformat see issue: #252 (closed)

So I don't know if we should have a specific forwardDType but I think it is a lot of different issues that are related to this need of passing more tensor information down the graph.

Can we issue a discussion on this matter asap?

Edit:

I would be in favor to add forwardDType forwardFormat and forwardDims and reunite all of them in forwardTensorProperties

@hrouis FYI