[BYOC][ACL] Fix list is not supported as an input node #10801

DzAvril · 2022-03-28T08:24:41Z

The input of op concatenate is a list of tensors. We tried to bring it to ACL and found the JSON node of it is like below.

{
  "nodes": [
    {
      "op": "input",
      "name": "arm_compute_lib_0_i0",
      "attrs": {
        "dtype": [
          [
            "int32",
            "int32",
            "int32"
          ]
        ],
        "shape": [
          [
            [1, 234, 234, 256],
            [1, 234, 234, 256],
            [1, 234, 234, 256]
          ]
        ]
      }
    },
    {
      "op": "kernel",
      "name": "concatenate",
      "inputs": [[
          0,
          0,
          0], [
          0,
          1,
          0], [
          0,
          2,
          0]],
      "attrs": {
        "num_outputs": "1",
        "num_inputs": "3",
        "dtype": [
          [
            "int32"
          ]
        ],
        "axis": [
          [
            "0"
          ]
        ],
        "shape": [
          [
            [3, 234, 234, 256]
          ]
        ]
      }
    }
  ],
  "arg_nodes": [0],
  "heads": [[
      1,
      0,
      0]],
  "node_row_ptr": [0, 3, 4]
}

The inputs of node kernel is:

"inputs": [[
          0,
          0,
          0], [
          0,
          1,
          0], [
          0,
          2,
          0]]

Element in inputs is the index of input node. Taking [0, 1, 0] as an example, the first 0 is nodeid, the second 1 is indexid, and the last 0 is version.
But in function Run, it only uses 0 as indexid in statement uint32_t eid = EntryID(nid, 0); so the other two inputs are ignored.

  void Run() override {
    for (size_t i = 0; i < input_nodes_.size(); ++i) {
      auto nid = input_nodes_[i];
      uint32_t eid = EntryID(nid, 0);
      if (nodes_[nid].GetOpType() == "input") {
        void* data = data_entry_[eid]->data;
        CheckACLError(layer_.inputs[i].allocator()->import_memory(data));
      }
    }

    for (size_t i = 0; i < outputs_.size(); ++i) {
      uint32_t eid = EntryID(outputs_[i]);
      void* data = data_entry_[eid]->data;
      CheckACLError(layer_.outputs[i].allocator()->import_memory(data));
    }

    this->layer_.function->run();
  }

This PR introduces a variable json_inputid_to_layer_inputid which maps the input index of JSON node to the index of the ACL layer's inputs.

u99127 · 2022-03-28T08:53:14Z

@lhutton1 - could you take a look ?

lhutton1

Thanks @DzAvril, its great to see more support being added to ACL again :) Apologies for the delayed review I was on holiday the past couple of days. Overall LGTM! The comments are mostly just about cleaning things up, testing and tightening the conditions under which concatenate gets offloaded

tests/python/contrib/test_arm_compute_lib/test_concatenate.py

python/tvm/relay/op/contrib/arm_compute_lib.py

src/runtime/contrib/arm_compute_lib/acl_runtime.cc

tests/python/contrib/test_arm_compute_lib/test_concatenate.py

src/runtime/contrib/arm_compute_lib/acl_runtime.cc

DzAvril · 2022-03-31T10:35:36Z

Thanks @DzAvril, its great to see more support being added to ACL again :) Apologies for the delayed review I was on holiday the past couple of days. Overall LGTM! The comments are mostly just about cleaning things up, testing and tightening the conditions under which concatenate gets offloaded

@lhutton1 Thanks for the comments. Please help to review again.

lhutton1 · 2022-03-31T15:28:02Z

Thanks @DzAvril LGTM!

cc @manupa-arm @comaniac @Mousius who may be able to help get this merged

DzAvril · 2022-04-07T08:23:38Z

Thanks @DzAvril LGTM!

cc @manupa-arm @comaniac @Mousius who may be able to help get this merged

Hi @lhutton1 , thanks for your review. As you have approved these changes, the status of this PR is supposed to be Approved, but in fact, it is still Review required. And quote:

Merging can be performed automatically with 1 approving review.

Will it be merged automatically after the status changes to Approved?

lhutton1 · 2022-04-07T08:29:42Z

Unfortunately since I'm just a reviewer my approval doesn't unblock merging, we would need to wait for someone who is a committer to approve this change and merge it.

Friendly ping @manupa-arm @comaniac @Mousius (I'm not sure who else might be interested in ACL support)

lhutton1 · 2022-04-10T14:22:13Z

Hi @DzAvril, I was having a think about this over the weekend and remembered that for the ACL integration we don't run the MergeCompilerRegions pass, meaning each operation is offloaded to ACL separately. Because of this, I'm not sure offloading concatenate (as opposed to running on TVM) would bring any performance improvement - since the operation doesn't really do any computation. So I'm curious, does this PR fix a separate issue you were seeing, or did you observe any kind of performance improvement while offloading concatenate?

My apologies for missing this previously - it's been a while since working on this.

DzAvril · 2022-04-11T01:09:15Z

Our team is working on a project which needs hot-upgrade for models, our approach is offloading as many operations as possible to ACL and performance is not a concern. So this PR fixes a separate issue of offloading operations which have a list as inputs and offloading concatenate is a typical case of this issue.

cee1 · 2022-04-11T04:44:12Z

which have a list as inputs and offloading concatenate is a typical case of this issue.
For src/runtime/contrib/json/json_runtime.h

The JSONRuntimeBase misses a line for multiple EntryID (eid), as the patch pointed out.

( And this issue hits us while hooking ACL concat for a feature of "hot upgradable model":

"Hot upgradable model" are pure data, no executable code within it.
Which desire a common Op library to be used by ALL models.

We leverage ACL as the base of the "common Op library". )

Hi @lhutton1, is it a more appropriate way to
a) Disable hooking to ACL' concat within relay/op/contrib/arm_compute_lib.py? (While keeping the concat entry in runtime/contrib/arm_compute_lib/acl_runtime.cc)

Or b) Submit the fixup of json_runtime.h only?

Any idea?

lhutton1 · 2022-04-11T08:43:17Z

Thanks for the insight @DzAvril, @cee1, I think in that case it would be fine to keep the implementation. It might be a good idea to disable offloading concatenate by default, but then add an optional parameter say offload_concat=False to partition_for_arm_compute_lib to manually turn on partitioning for concatenate, WDYT?

cee1 · 2022-04-12T01:25:31Z

offload_concat=False to partition_for_arm_compute_lib to manually turn on partitioning for concatenate

Hi @lhutton1, would you please review this revision of the patch?

lhutton1

Thanks for the revision @DzAvril, @cee1. LGTM.. again!

cee1 · 2022-04-12T11:36:36Z

Thanks for the revision @DzAvril, @cee1. LGTM.. again!

Hi @u99127, could you review this patch and perform "approving review" if looks fine ...?

lhutton1 · 2022-04-13T08:36:10Z

@masahi @areusch @tmoreau89 - we're struggling to find a committer to take a look at this PR, do you have any suggestions?

masahi · 2022-04-13T08:49:56Z

python/tvm/relay/op/contrib/arm_compute_lib.py


    Returns
    -------
    ret : annotated and partitioned module.
    """
+    global offload_concat_


Rather than using a global var, can we make use of arm_compute_lib_pattern_table?

tvm/python/tvm/relay/op/contrib/arm_compute_lib.py

Line 131 in 8a0472f

def arm_compute_lib_pattern_table():

i.e. register concat pattern if offload_concat == True.

Thanks for taking a look @masahi. Currently ACL has a mixture of registering operations using composite functions and _register_external_op_helper which adds the target.arm_compute_lib attribute, which is not ideal. I think if we wanted to do this for concat it would need to be registered using pattern table rather than the other mechanism.

In the future we should probably use the pattern table for all operations.

Yeah, if we are adding a new op support, why not add it via pattern like

tvm/python/tvm/relay/op/contrib/arm_compute_lib.py

Line 134 in 8a0472f

def conv_pattern():

Then it is easy to enable / disable a pattern based on some flags. @DzAvril

Updated, please review. I'm not sure it is what you want because it seems a little weird since arm_compute_lib_pattern_table aims to register fusing patterns for ops and registering op attr is out of it.
BTW, this commit triggered a Sanity Check error:

python/tvm/relay/op/contrib/arm_compute_lib.py:274: [W0612(unused-variable), arm_compute_lib_pattern_table.concatenate] Unused variable 'concatenate'

Actually I don't know what @tvm.ir.register_op_attr("nn.conv2d", "target.arm_compute_lib") thing is for... Why do you need both the pattern + check function at

tvm/python/tvm/relay/op/contrib/arm_compute_lib.py

Line 134 in 8a0472f

def conv_pattern():

tvm/python/tvm/relay/op/contrib/arm_compute_lib.py

Line 223 in 8a0472f

def check_conv(extract):

and register_op_attr thing at

tvm/python/tvm/relay/op/contrib/arm_compute_lib.py

Lines 291 to 292 in 8a0472f

@tvm.ir.register_op_attr("nn.conv2d", "target.arm_compute_lib")

def conv2d(expr):

?

I never needed the latter in the BYOCs I worked on.

Actually I don't know what @tvm.ir.register_op_attr("nn.conv2d", "target.arm_compute_lib") thing is for...

Gently ping @lhutton1, can you help to explain this to masai?

@DzAvril, @lhutton1 said "I think if we wanted to do this for concat it would need to be registered using pattern table rather than the other mechanism". This is also what I'm suggesting above. Can you do that? You shouldn't need @tvm.ir.register_op_attr thing for concat. You just need to add is_op("concatenate") etc + check function which you already have under @tvm.ir.register_op_attr(concatenate)

Actually I don't know what @tvm.ir.register_op_attr("nn.conv2d", "target.arm_compute_lib")

Yeah I don't think we need to register conv2d like this either as it should have already been picked up by the pattern table. This way of registering operators predates the pattern table so I guess this was just overlooked. We can follow up in a separate PR to move all operators so that they are registered using the pattern table - which I believe is the way other BYOC's are going. Apologies for the confusion @DzAvril, like @masahi said, it would be best to register concatenate (and other operators added in the future) using the pattern table since this is the preferred mechanism.

Sorry for missing some comments above. I have a question about the concatenate_pattern, see code below.

def concatenate_pattern(): """Create an concatenate pattern from equivalent relay operators. Returns ------- pattern : dataflow_pattern.AltPattern Denotes the concatenate pattern. """ pattern = is_op("concatenate")(wildcard()) return pattern

As the input of concatenate is a tuple of tensors, what the pattern of the parameters should be like? wildcard() in the code block is not working.

can you try is_tuple(None)? See #7754

DzAvril · 2022-04-13T14:39:45Z

@masahi @lhutton1 Thanks for your comments. I re-support concatenate with pattern table mechanism and bring two changes.

Add a parameter called disabled_ops that indicates the ops we do not offload to ACL to partition_for_arm_compute_lib, concatenate is included by default.
Add a function called AddCommonSingleJSONNode to add single op like concatenate to graph.

  std::vector<JSONGraphNodeEntry> AddCommonSingleJSONNode(const CallNode* cn, std::string name) {
    std::vector<JSONGraphNodeEntry> inputs;
    for (const auto& arg : cn->args) {
      auto res = VisitExpr(arg);
      inputs.insert(inputs.end(), res.begin(), res.end());
    }
    auto node = std::make_shared<JSONGraphNode>(name,     /* name_ */
                                                "kernel", /* op_type_ */
                                                inputs, 1 /* num_outputs_ */);

    const auto* fn = cn->op.as<FunctionNode>();
    ICHECK(fn);
    const auto* callNode = fn->body.as<CallNode>();
    ICHECK(callNode);
    SetCallNodeAttribute(node, callNode);
    return AddNode(node, GetRef<Expr>(cn));
  }

This function calls AddNode which returns a list of graph entry nodes and flattens the tuple type expr. In this way, the core problem this PR aims to fix is gone, because after flattening tuple type expr the JSON graph like below:

  {
    "attrs": {
      "dtype": [
        [
          "float32"
        ]
      ],
      "shape": [
        [
          [
            1,
            234,
            234,
            256
          ]
        ]
      ]
    },
    "name": "",
    "op": "input"
  },
  {
    "attrs": {
      "dtype": [
        "float32"
      ],
      "shape": [
        [
          2,
          234,
          234,
          256
        ]
      ]
    },
    "name": "",
    "op": "input"
  },
  {
    "attrs": {
      "dtype": [
        "float32"
      ],
      "shape": [
        [
          3,
          234,
          234,
          256
        ]
      ]
    },
    "name": "",
    "op": "input"
  },
  {
    "attrs": {
      "axis": [
        [
          "0"
        ]
      ],
      "dtype": [
        [
          "float32"
        ]
      ],
      "num_inputs": "3",
      "num_outputs": "1",
      "shape": [
        [
          [
            6,
            234,
            234,
            256
          ]
        ]
      ]
    },
    "inputs": [
      [
        0,
        0,
        0
      ],
      [
        1,
        0,
        0
      ],
      [
        2,
        0,
        0
      ]
    ],
    "name": "concatenate",
    "op": "kernel"
  }

It will work without json_inputid_to_layer_inputid. Even though I think we should keep the change I made in acl_runtime.cc and other related files in case of other corner cases occur.

DzAvril · 2022-04-13T14:42:45Z

def arm_compute_lib_pattern_table(disabled_ops=["concatenate"]):

The default value of disabled_ops triggers an error in Sanity Check

Dangerous default value [] as argument

I have no idea how to fix this, can you give a hint pls?

masahi · 2022-04-13T23:08:59Z

You can add dangerous-default-value to

tvm/python/tvm/relay/op/contrib/arm_compute_lib.py

Line 17 in 8a0472f

# pylint: disable=invalid-name, unused-argument

to suppress this warning.

* [BYOC][ACL] Fix list is not supported as an input node * fix clang lint error * fix compile warnning * fix python module import error * rename concatenate test file * fix always MakeACLTensor with same eid 0 * do not offload concat default * fix concattnate test failure * fix test failure * fix lint error * fix lint * remove global var offload_concat * support concatenate with pattern table mechanism * disable pylint dangerous-default-value warning Co-authored-by: XuZhi <xuzhi.xu@alibaba-inc.com>

XuZhi added 2 commits March 28, 2022 18:08

[BYOC][ACL] Fix list is not supported as an input node

5845760

fix clang lint error

937e91a

DzAvril force-pushed the alc-dev branch from 2ee4a97 to 937e91a Compare March 28, 2022 10:09

XuZhi added 3 commits March 28, 2022 18:38

fix compile warnning

b47f26a

fix python module import error

4089f17

rename concatenate test file

1d3aebc

lhutton1 reviewed Mar 30, 2022

View reviewed changes

fix always MakeACLTensor with same eid 0

d89b7d4

lhutton1 approved these changes Mar 31, 2022

View reviewed changes

XuZhi added 4 commits April 11, 2022 19:20

do not offload concat default

8c87d45

fix concattnate test failure

6c03b19

fix test failure

9d36318

fix lint error

ca28694

fix lint

9bb6421

lhutton1 approved these changes Apr 12, 2022

View reviewed changes

masahi reviewed Apr 13, 2022

View reviewed changes

XuZhi added 2 commits April 13, 2022 17:18

remove global var offload_concat

56eb714

support concatenate with pattern table mechanism

b219357

disable pylint dangerous-default-value warning

4603c12

masahi approved these changes Apr 14, 2022

View reviewed changes

masahi merged commit 52f52c8 into apache:main Apr 14, 2022

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BYOC][ACL] Fix list is not supported as an input node #10801

[BYOC][ACL] Fix list is not supported as an input node #10801

DzAvril commented Mar 28, 2022

u99127 commented Mar 28, 2022

lhutton1 left a comment

DzAvril commented Mar 31, 2022

lhutton1 commented Mar 31, 2022

DzAvril commented Apr 7, 2022

lhutton1 commented Apr 7, 2022

lhutton1 commented Apr 10, 2022

DzAvril commented Apr 11, 2022

cee1 commented Apr 11, 2022

lhutton1 commented Apr 11, 2022

cee1 commented Apr 12, 2022

lhutton1 left a comment

cee1 commented Apr 12, 2022

lhutton1 commented Apr 13, 2022

masahi Apr 13, 2022 •

edited

Loading

lhutton1 Apr 13, 2022

masahi Apr 13, 2022 •

edited

Loading

DzAvril Apr 13, 2022

masahi Apr 13, 2022 •

edited

Loading

DzAvril Apr 13, 2022

masahi Apr 13, 2022 •

edited

Loading

lhutton1 Apr 13, 2022

DzAvril Apr 13, 2022

masahi Apr 13, 2022

DzAvril commented Apr 13, 2022

DzAvril commented Apr 13, 2022

masahi commented Apr 13, 2022

	@tvm.ir.register_op_attr("nn.conv2d", "target.arm_compute_lib")
	def conv2d(expr):

[BYOC][ACL] Fix list is not supported as an input node #10801

[BYOC][ACL] Fix list is not supported as an input node #10801

Conversation

DzAvril commented Mar 28, 2022

u99127 commented Mar 28, 2022

lhutton1 left a comment

Choose a reason for hiding this comment

DzAvril commented Mar 31, 2022

lhutton1 commented Mar 31, 2022

DzAvril commented Apr 7, 2022

lhutton1 commented Apr 7, 2022

lhutton1 commented Apr 10, 2022

DzAvril commented Apr 11, 2022

cee1 commented Apr 11, 2022

lhutton1 commented Apr 11, 2022

cee1 commented Apr 12, 2022

lhutton1 left a comment

Choose a reason for hiding this comment

cee1 commented Apr 12, 2022

lhutton1 commented Apr 13, 2022

masahi Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

lhutton1 Apr 13, 2022

Choose a reason for hiding this comment

masahi Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

DzAvril Apr 13, 2022

Choose a reason for hiding this comment

masahi Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

DzAvril Apr 13, 2022

Choose a reason for hiding this comment

masahi Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

lhutton1 Apr 13, 2022

Choose a reason for hiding this comment

DzAvril Apr 13, 2022

Choose a reason for hiding this comment

masahi Apr 13, 2022

Choose a reason for hiding this comment

DzAvril commented Apr 13, 2022

DzAvril commented Apr 13, 2022

masahi commented Apr 13, 2022

masahi Apr 13, 2022 •

edited

Loading

masahi Apr 13, 2022 •

edited

Loading

masahi Apr 13, 2022 •

edited

Loading

masahi Apr 13, 2022 •

edited

Loading