Samsung / ONE

On-device Neural Engine
Other
429 stars 157 forks source link

[nncc] Assign input's Shape in Circle<op>Out node instance #3757

Open dr-venkman opened 4 years ago

dr-venkman commented 4 years ago

ShapeInference for unique's output index needs to follow from it's input tensor, as per definition. From the draft pull request in #2829, this is expected to happen at the function loco::NodeShape visit(const luci::CircleUniqueOut *node) {.. }.

However, I am not able to access the input shape as seamlessly as I was able to do so with loco::NodeShape visit(const luci::CircleUnique *node), where auto input_shape = loco::shape_get(node->input()).as<loco::TensorShape>(); fetched me the input shape without issues.

In loco::NodeShape visit(const luci::CircleUniqueOut *node) {.. }, please refer to the following lines as reference:

    auto unique = dynamic_cast<const luci::CircleUnique *>(node->input());
    if (unique == nullptr)
    {
      INTERNAL_EXN("CircleUnique IR is not configured correctly");
    }

    auto unique_shape = loco::shape_get(unique).as<loco::TensorShape>();

Here, unique_shape is not what I was expecting, i.e. the input operand shape. My question in general is whether it is indeed possible to access the input shape from within the above function. Kindly let me know. Thank you in advance.

seanshpark commented 4 years ago

All other nodes input shapre reference uses codes like

auto unique_shape = loco::shape_get(unique).as<loco::TensorShape>();

not what I was expecting,

what do you exect and what is the value you get? how did you get that value?

dr-venkman commented 4 years ago

All other nodes input shapre reference uses codes like

auto unique_shape = loco::shape_get(unique).as<loco::TensorShape>();

not what I was expecting,

what do you exect and what is the value you get? how did you get that value?

I was expecting to retreive the input shape and assign it's rank and dim[0] to an output shape, just as I had done previously inside loco::NodeShape visit(const luci::CircleUnique *node).

In this case, however, unique_shape does not have a well-defined _dims vector that i can access and assign values from. In the former case, input_shape had rank() == 1, and had the dimension values set within _dims.

seanshpark commented 4 years ago

i can access and assign values from

where is from ?

seanshpark commented 4 years ago

Can you be more specific to real numbers? I can't catch what exactly the problem is.

dr-venkman commented 4 years ago

@seanshpark, I am currently testing with the following recipe:

operand {
  name: "ifm"
  type: FLOAT32
  shape { dim: 4 }
}
operand {
  name: "ofm"
  type: FLOAT32
  shape { }
}
operand {
  name: "ofm_idx"
  type: INT64
  shape { dim: 4 }
}
operation {
  type: "Unique"
  unique_options {
    idx_out_type: INT64
  }
  input: "ifm"
  output: "ofm"
  output: "ofm_idx"
}
input: "ifm"
output: "ofm"
output: "ofm_idx"

At loco::NodeShape visit(const luci::CircleUniqueOut *node) {..}, the line auto unique = dynamic_cast<const luci::CircleUnique *>(node->input()); the dynamic casting reveals a CircleNode whose _idx_out_type is loco::DataType::S64, the same as that of operand ofm_idx in the recip[e above. Hence my reasoning that we need to assign a rank = 1, dim(0) = 4 here.

This means, I need to access the input operand. However, the line auto unique_shape = loco::shape_get(unique).as<loco::TensorShape>(); does not fetch me the input shape with the dimensions as mentioned above, in fact, it appears to be undefined as to its values for _dims.

As a next step, I started comparing addresses, and I find unique_shape to be of type CircleNode * and having a address value of 0x555555778a20. This matches both the type and value of input_shape at loco::NodeShape visit(const luci::CircleUnique *node). So, as a correction, the following change could work:

Replace what is currently

auto unique_shape = loco::shape_get(unique).as<loco::TensorShape>();

with

auto unique_shape = loco::shape_get(unique->input()).as<loco::TensorShape>();

I have not built and tested this change yet, but at least, it seems consistent with the values that I see (described above). As this approach deviates from what you mentioned, please do comment on whether this could work or is outright wrong. Thanks.

seanshpark commented 4 years ago

I'm not sure I understand your problem...

Let's see how the shape inference runs... Can you put a LOG or just printf in both CircleUnique and CircleUniqueOut with the shape values ?

dr-venkman commented 4 years ago

@seanshpark , I passed the tests with the code changes as described above. Let me reply pointwise to your remarks:

in visit(const luci::CircleUniqueOut *node) method, node->input() which is Unique node has not been shape inferenced yet.

Yes, node is an instance of CircleUniqueOut, and fetching it's input does give a shape that is unknown. In contrast, unique is of type CircleUnique *, and fetching it's input is exactly what I wanted to do. Therefore the changes as mentioned above worked.

shape inference should run in order of CircleUnique and then CircleUniqueOut Yes, I verified that via gdb.

Please do let me know if you suggest a different approach to what I mentioned above.

seanshpark commented 4 years ago

Therefore the changes as mentioned above worked.

I'm not sure I would accept current implementation.

I've looked at https://www.tensorflow.org/api_docs/python/tf/unique page and looked at your draft code again. This time thinking about how I would implement for this op. In your draft code, type inference and shape inference differs. why are they different?

What is(are) the difference of CircleUnique and CircleUniqueOut, What is CircleUniqueOut needed for? What is unique_out->index() for?

Can you explain these? Not that you should expain these in the comments, but do you really understand them?

dr-venkman commented 4 years ago

@seanshpark ,

Please find my replies below:

In your draft code, type inference and shape inference differs. why are they different?

At the time it was implemented, I borrowed the coding conventions from other operations such as UnpackOut and SplitOut. I agree the type and shape inferencing needs to follow a similar pattern of checking output index. However, the larger question in context is why the UniqueOut node is visited only once (as seen from debugging sessions). I will outline my findings further down this post.

What is(are) the difference of CircleUnique and CircleUniqueOut, What is CircleUniqueOut needed for? What is unique_out->index() for?

From our offline discussion, I gathered that CircleUnique and CircleUniqueOut represent graph nodes, which should be visited in the order i. CircleUnique, ii. CircleUniqueOut, indx 1, and iii. CircleUniqueOut, indx 0. The order stated here is what I see from my debugging breakpoints. The reverse order of CircleUniqueOut could be a result of a post-order traversal of nodes, as outlined in compiler/loco/src/Service/ShapeInference.cpp, function ShapeInferenceSession::to(Graph *g).

Now, on the subject of the CircleUniqueOuts, I observed the following:

  1. Type Inferencing visit both the UniqueOut nodes, so the index check serves well to disambiguate them

  2. Shape Inferencing visits only the UniqueOut nodes corresponding to ofm_idx and not ofm.

Concerning point 2 above, I noticed that the circle node corresponding to ofm, has a rank 0, as it is not yet defined. This results to setting is_scalar to true in compiler/luci/service/src/CircleShapeInferenceRule.cpp, function bool CircleShapeInferenceRule::infer(const loco::Node *node, loco::NodeShape &shape) const. As the output shape is valid, and the rank is 0, the following condition evaluates to true:

 if (is_shape_none || is_scalar)
      shape = own_shape(circle_node);

The result, is that for ofm, the shape is assigned from the specification, and the testcases do pass, as I mentioned before.

In light of the above explanation, I understand that the correct approach is to visit the UniqueOut node for ofm explicitly. However, in order to do so, it's rank must be a value other than 0, and as per specification, it must be 1. However, I am not sure at what stage I can enforce a rank assignment of 1. Could you please share your thoughts on this? Thanks.

seanshpark commented 4 years ago

it's rank must be a value other than 0, and as per specification, it must be 1

From https://www.tensorflow.org/api_docs/python/tf/unique

From this, the recipe in https://github.com/Samsung/ONE/pull/2829/files#diff-e58de95e2f4f273e38f04c43037af5e0

operand {
  name: "ofm"
  type: FLOAT32
  shape { }
}

should be

operand {
  name: "ofm"
  type: FLOAT32
  shape { dim: 4 }
}
 if (is_shape_none || is_scalar)
      shape = own_shape(circle_node);

logic is to handle unknown shape. as ofm is shape {}, this is interpreted as unknown.

dr-venkman commented 4 years ago

it's rank must be a value other than 0, and as per specification, it must be 1

From https://www.tensorflow.org/api_docs/python/tf/unique

  • x A Tensor. 1-D.
  • y A Tensor. Has the same type as x.

From this, the recipe in https://github.com/Samsung/ONE/pull/2829/files#diff-e58de95e2f4f273e38f04c43037af5e0

operand {
  name: "ofm"
  type: FLOAT32
  shape { }
}

should be

operand {
  name: "ofm"
  type: FLOAT32
  shape { dim: 4 }
}
 if (is_shape_none || is_scalar)
      shape = own_shape(circle_node);

logic is to handle unknown shape. as ofm is shape {}, this is interpreted as unknown.

Thank you for the reply. I understand that we assign a rank of 1 by assigning shape {dim: 4} to ofm (representing the output y). Strictly speaking though, its dimensions may not equal 4. Is this a issue, or an artifact of the implementation? Could you please let me know?

seanshpark commented 4 years ago

, its dimensions may not equal 4.

Ah.. yes.. this is the one that we need to solve.

Is this a issue, or an artifact of the implementation?

Yes, we need to solve this somehow, if current implementation doesn't support this feature.

For unknown, could you try with solving with this?

operand {
  name: "ofm"
  type: FLOAT32
  shape { dim: -1 }
}
dr-venkman commented 4 years ago

@seanshpark ,

I tried as suggested, but got the following error:

[ 41%] Generating Unique_000.recipe
[ 41%] Generating Unique_000.tflite
[libprotobuf ERROR $PROJECT_FOLDER/externals/PROTOBUF/src/google/protobuf/text_format.cc:288] Error parsing text-format tflchef.ModelRecipe: 9:16: Expected integer, got: -
ERROR: Failed to parse recipe 'Unique_000.recipe'
compiler/tflchef/tests/CMakeFiles/tflchef_testfiles.dir/build.make:1438: recipe for target 'compiler/tflchef/tests/Unique_000.tflite' failed
make[2]: *** [compiler/tflchef/tests/Unique_000.tflite] Error 255
CMakeFiles/Makefile2:15025: recipe for target 'compiler/tflchef/tests/CMakeFiles/tflchef_testfiles.dir/all' failed
make[1]: *** [compiler/tflchef/tests/CMakeFiles/tflchef_testfiles.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2

Apparently, -1 does not parse as valid. Please correct me if I overlook something.

seanshpark commented 4 years ago

Apparently, -1 does not parse as valid.

Um... seems not implemented yet

1) implement to accept -1 2) try with

operand {
  name: "ofm"
  type: FLOAT32
  shape { dim: 0 }
}