One: Let's support TFL schema 3a(Tensorflow 2.4)

[chunseoklee]

Let's discuss and share issues about TF Lite's 3a schema support.

Notice that builtin op type is int32 now :

https://github.com/tensorflow/tensorflow/blob/a0afe8f9218be5eb9ed5dffc2dff652996da8c28/tensorflow/lite/schema/schema.fbs#L1061-L1077

table OperatorCode {
  // This field is for backward compatibility. This field will be used when
  // the value of the extended builtin_code field has less than
  // BulitinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES.
  deprecated_builtin_code:byte;
  custom_code:string;


  // The version of the operator. The version need to be bumped whenever new
  // parameters are introduced into an op.
  version:int = 1;


  // This field is introduced for resolving op builtin code shortage problem
  // (the original BuiltinOperator enum field was represented as a byte).
  // This field will be used when the value of the extended builtin_code field
  // has greater than BulitinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES.
  builtin_code:BuiltinOperator;
}

[seanshpark]

5688 for compiler frontend :)

[hseok-oh]

My suggestion to handle circle's BuiltinOperator for TFLite schema v3a

• Enum data type ubyte -> int32: same with TFLite schema v3a
• ADD(0) ~ BATCH_MATMUL(126): same number
• BCQ_GATHER (252) ~ INSTANCE_NORM (254)
  
  
  
  • Change number, but use same LSB 8bit
  
  
  • ex) BCQ_GATHER: 252 - 2^12 = -3844
  
  
• New OPs introduced on TFLite: same number
• New OPs introduced on circle: use negative number

[seanshpark]

use negative number --> how about... 252 == 0xFC --> 0x7FFFFFFC for int32 ?

[seanshpark]

There's a code in schema_generated.h where

inline const char * const *EnumNamesBuiltinOperator() {
  static const char * const names[] = {
    "ADD",
    ...
    "",
    "",
    "BCQ_GATHER",
    "BCQ_FULLY_CONNECTED",
    "INSTANCE_NORM",
    nullptr
  };
  return names;
}

if we use a number quite huge then there will be very long emtpy string ""," items... which would be a bit ugly...

[hseok-oh]

I've tested with negative number

enum BuiltinOperator : int32 {
  BCQ_GATHER = -3,
  BCQ_FULLY_CONNECTED = -2,
  INSTANCE_NORM = -1,
  ADD = 0,
  AVERAGE_POOL_2D = 1,
  CONCATENATION = 2,
  CONV_2D = 3,
  DEPTHWISE_CONV_2D = 4,
  ...
  BATCH_MATMUL = 126,
}

then schema_generated.h

enum BuiltinOperator {
  BuiltinOperator_BCQ_GATHER = -3,
  BuiltinOperator_BCQ_FULLY_CONNECTED = -2,
  BuiltinOperator_INSTANCE_NORM = -1,
  BuiltinOperator_ADD = 0,
  BuiltinOperator_AVERAGE_POOL_2D = 1,
  BuiltinOperator_CONCATENATION = 2,
  ...
  BuiltinOperator_BATCH_MATMUL = 126,
  BuiltinOperator_MIN = BuiltinOperator_BCQ_GATHER,
  BuiltinOperator_MAX = BuiltinOperator_BATCH_MATMUL
};

inline const char * const *EnumNamesBuiltinOperator() {
  static const char * const names[] = {
    "BCQ_GATHER",
    "BCQ_FULLY_CONNECTED",
    "INSTANCE_NORM",
    "ADD",
    "AVERAGE_POOL_2D",
    "CONCATENATION",
    "CONV_2D",
    "DEPTHWISE_CONV_2D",
    ...
    "BATCH_MATMUL",
    nullptr
  };
  return names;
}

inline const char *EnumNameBuiltinOperator(BuiltinOperator e) {
  const size_t index = static_cast<int>(e) - static_cast<int>(BuiltinOperator_BCQ_GATHER);
  return EnumNamesBuiltinOperator()[index];
}

We can avoid large size name array and ugly code.

[glistening]

For your information, using negative for extended operators has another advantage:

If we use number greater than INT32_MAX, we would get flatc error like old internal #8451 without changing enum BuiltinOperator to enum BuiltinOperator : uint32.

[llFreetimell]

Currently, TFLite has following policy.

• When BuiltinOperator is less than PLACEHOLDER_FOR_GREATER_OP_CODES(127)
  
  
  
  • deprecated_builtin_code and builtin_code is same.
  
  
  • For example, if op is SUB :
    
    table OperatorCode { deprecated_builtin_code:byte; // 41(BuiltinOperator_SUB) custom_code:string; version:int = 1; builtin_code:BuiltinOperator; // BuiltinOperator_SUB(41) }
  
  
• When BuiltinOperator is greater than PLACEHOLDER_FOR_GREATER_OP_CODES(127)
  
  
  
  • deprecated_builtin_code is set to BuiltinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES
  
  
  • For example, if op is CUMSUM(128) :
    
    table OperatorCode { deprecated_builtin_code:byte; // 127(BuiltinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES) custom_code:string; version:int = 1; builtin_code:BuiltinOperator; // BuiltinOperator_CUMSUM(128) }
  
  

So following rules can be applied in TFLite for import and export

• If deprecated_builtin_code is less than BuiltinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES
  
  
  
  • Use deprecated_builtin_code
  
  
• If deprecated_builtin_code is BuiltinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES
  
  
  
  • Use builtin_code
  
  

---

This is natural for TFLite because there are no more ops after PLACEHOLDER_FOR_GREATER_OP_CODES in v2.3.
However, for circle, it can cause some problems in the future because of following operators.

• BCQ_GATHER (252)
• BCQ_FULLY_CONNECTED (253)
• INSTANCE_NORM (254)

So I want to suggest following rules for circle
(For more details, please refer to circle_schema.fbs in #6269)

• Circle Schema change
  
  
  
  • Introduce enum DeprecatedBuiltinOperator : ubyte for keeping deprecated builtin code.
    
    cpp enum DeprecatedBuiltinOperator : ubyte { ADD = 0, AVERAGE_POOL_2D = 1, ... BATCH_MATMUL = 126, PLACEHOLDER_FOR_GREATER_OP_CODES = 127, BCQ_GATHER = 252, BCQ_FULLY_CONNECTED = 253, INSTANCE_NORM = 254, }
  
  
  • Introcuce enum BuiltinOperator : int32 for following new TFLite schema
    
    cpp enum BuiltinOperator : int32 { BCQ_GATHER = -3, BCQ_FULLY_CONNECTED = -2, INSTANCE_NORM = -1, ADD = 0, AVERAGE_POOL_2D = 1, ... BATCH_MATMUL = 126, PLACEHOLDER_FOR_GREATER_OP_CODES = 127, CUMSUM = 128, }
  
  
• Interpretation rule for import
  
  
  
  • If deprecated_builtin_code == PLACEHOLDER_FOR_GREATER_OP_CODES
  
  
  • Use deprecated_builtin_code
  
  
  • If deprecated_builtin_code != PLACEHOLDER_FOR_GREATER_OP_CODES
  
  
  • Use builtin_code
  
  
• Interpretation rule for export (Maybe only related with compiler frontend)
  
  
  
  • Always set deprecated_builtin_code to PLACEHOLDER_FOR_GREATER_OP_CODES
  
  
  • Set builtin_code to proper BuiltinOperator
  
  

[hseok-oh]

If we define deprecated_builtin_code field as byte type, we don't need to introduce DeprecatedBuiltinOperator enum.

[llFreetimell]

@hseok-oh

I was to modify as following schema :)

table OperatorCode {
  // This field is for backward compatibility. This field will be used when
  // the value of the extended builtin_code field has less than
  // BulitinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES.
  deprecated_builtin_code:DeprecatedBuiltinOperator;
  custom_code:string;

  // The version of the operator. The version need to be bumped whenever new
  // parameters are introduced into an op.
  version:int = 1;

  // This field is introduced for resolving op builtin code shortage problem
  // (the original BuiltinOperator enum field was represented as a byte).
  // This field will be used when the value of the extended builtin_code field
  // has greater than BulitinOperator_PLACEHOLDER_FOR_GREATER_OP_CODES.
  builtin_code:BuiltinOperator;
}

And there is another reason to do so, operator 253~255 may be conflicted in the future :(

[hseok-oh]

operator 253~255 may be conflicted in the future :(

If we use byte instead of ubyte, and mapping 253 -> -3 / 254 -> -2 / 255 -> -1, then we can avoid conflict without value change.

[llFreetimell]

@hseok-oh I will try with this :)

[llFreetimell]

I checked it works well, thanks :)
We can follow TFLite rule!

[seanshpark]

@llFreetimell , onnx-tensorflow up to come 1.8.0 version will be referencing TF 2.4.1

• https://github.com/onnx/onnx-tensorflow/pull/893/files

[llFreetimell]

onnx-tensorflow up to come 1.8.0 version will be referencing TF 2.4.1

Then I will proceed with TF 2.4.1 schema!
(https://github.com/tensorflow/tensorflow/blob/r2.4/tensorflow/lite/schema/schema.fbs)