One: Compiler FE: how to handle the type of CircleOutputExclude

[mhs4670go]

This issue is about handling the type of CircleOutputExclude IR.

The current implementation is as follows.

$ cat compiler/luci/import/src/GraphBuilder.cpp
...
std::vector<CircleNode *> input_nodes;
  for (const int32_t input_tensor_index : inputs)
  {
    if (input_tensor_index >= 0)
    {
      auto input = context->nodefinder()->node(input_tensor_index);
      if (input == nullptr)
        INFO(l) << "[luci] Warning: input node is null " << input_tensor_index << std::endl;
      input_nodes.push_back(input);
    }
    else
    {
      // If there is no tensor, insert CircleOutputExclude.
      input_nodes.push_back(context->graph()->nodes()->create<luci::CircleOutputExclude>()); // NOTE HERE
    }
  }
...

CircleOutputExclude doesn't need a type, but since all nodes must have a type, a dummy type is inserted.

Because of this, when we build a node that has tensor whose tensor index is -1, we've got to set dummy dtype.

$ cat compiler/luci/import/src/Nodes/CircleFullyConnected.cpp
...
// TODO Find and move to appropriate place for setting optional input
  if (auto bias = dynamic_cast<luci::CircleOutputExclude *>(node->bias()))
  {
    // bias is not used for type inference, but node itself should have a type
    bias->dtype(loco::DataType::FLOAT32);

    // bias is not used for shape inference
  }
...

I don't think it's efficient. It would be better to set dtype as soon as we create CircleOutputExclude node.

auto node = context->graph()->nodes()->create<luci::CircleOutputExclude>();
node->dtype(loco::DataType::FLOAT32);
input_nodes.push_back(node);

@llFreetimell Please give me some your opinion.

[llFreetimell]

At first, I had had two ideas.

• Set dtype when CircleOutputExclude is created (as you mentioned)
• Introducing loco::DataType::NONE

However, both of them was rejected at that time :(
I am glad for your opinion but other people's opinion seems important I think :)

[seanshpark]

However, both of them was rejected at that time :(

Do you remember the reason it was rejected?

[llFreetimell]

Set dtype when CircleOutputExclude is created

The reason was "Although current design is not best, setting dtype when CircleOutputExclude is created is not a proper way to handle it. So go as current design and think about better idea later."

Introducing loco::DataType::NONE

The reason was "Introducing new datatype in loco can influence other modules, codes and so on. Therefore if it is not the essential or inevitable solution, introducing new datatype in loco must be considered carefully."

[mhs4670go]

I think both of them look good. Furthermore, since all CircleOutputExclude's dtype is same, setting dtype in its constructor is another good option as well.

[seanshpark]

Question... does this hold for CircleFullyConnected is quantized ?

• setting node->dtype(loco::DataType::FLOAT32); at creation time holds that CircleFullyConnected bias dtype is ignored.

[mhs4670go]

@seanshpark Not for quantization. It's for passing validation whose condition checks if it has dtype, which means any type can be assigned. FYI, in quantization, bias must be CircleConst first.

[s-barannikov]

I'd just pass nullptr instead of CircleOutputExclude (which also has a misleading name), and forget about dtype and similar issues... :)

[mhs4670go]

@s-barannikov Actually, I kind of agree your point. I'm not sure but one of the reasons for this node was to distinguish nullptr that would be generated for optional input and nullptr for error or sth. I'll write a draft that uses nullptr rather than CircleOutputExclude later to see the possibilities. Or it would be nice if you write it when you have enough time.

[mhs4670go]

Also, FYI, when loco verifies nodes, there shouldn't be nullptr inputs.

void run(void) const
  {
    for (auto node : loco::all_nodes(_graph))
    {
      // Verify nodes
      for (uint32_t n = 0; n < node->arity(); ++n)
      {
        if (node->arg(n) == nullptr)
        {
          notify(ErrorDetail<ErrorCategory::MissingArgument>{node, n});
        }
      }
    }
  }

[s-barannikov]

Also, FYI, when loco verifies nodes, there shouldn't be nullptr inputs.

Yes, it does not support nullptrs as inputs by-design.
But if this is the only one or one of a few places that needs to be fixed, I think it is OK.

Or it would be nice if you write it when you have enough time.

I'd try, but not earlier than next week.