tvm icon indicating copy to clipboard operation
tvm copied to clipboard

[Array] Add deduction guide for Array(IterType,IterType) constructor

Open Lunderberg opened this issue 2 years ago • 8 comments
trafficstars

Previously, the Array(IterType,IterType) constructor required the class template parameter T to be explicitly provided. This commit adds a template deduction guide, specifying the intended type T as the type produced by de-referencing the iterator, to remove the requirement.

Fixes https://github.com/apache/tvm/issues/14600

Lunderberg avatar Apr 12 '23 14:04 Lunderberg

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: array See #10317 for details

Generated by tvm-bot

tvm-bot avatar Apr 12 '23 14:04 tvm-bot

@Lunderberg @Hzfengsy Thanks you for your advice and patch. However, I'm sorry to tell you that this crash still exists after using the patch mentioned in this PR. The build crash message:

/tvm/src/tir/analysis/identify_memcpy.cc:298:27: error: no viable constructor or deduction guide for deduction of template arguments of 'Array'
        output->push_back(Array{ptr->source, ptr->dest});

jikechao avatar Apr 18 '23 03:04 jikechao

@jikechao Could you please try to upgrade your cxx compiler?

Hzfengsy avatar Apr 18 '23 03:04 Hzfengsy

@Hzfengsy I have updated llvm-gcc-12.0 which is the latest version that is supported by Ubuntu16.04. But this bug exists. As a workaround, I modify the source code by replacing Array{ptr->source, ptr->dest} withArray<ObjectRef>{ptr->source, ptr->dest} to avoid the build failure.

jikechao avatar Apr 18 '23 15:04 jikechao

Interesting. While the change compiles locally, it looks like the CI is having issues with this patch, with the older version of gcc not interpreting the deduction guide correctly.

Looking at this specific use case again, it looks like it is also dependent on the ambiguity between Array{first_element, second_element} and Array{iter_begin, iter_end}. The deduction guides would help handle the case where the second usage has ambiguous type, but it wouldn't handle the case where the first usage is intended. We may be able to remove this ambiguity by moving the static_assert into a SFINAE parameter, so that the initializer_list case is the only remaining overload.

Can you try making the following change in include/tvm/runtime/container/array.h?

// Before
template <typename IterType>
Array(IterType first, IterType last) {
  static_assert(is_valid_iterator_v<T, IterType>,
                "IterType cannot be inserted into a tvm::Array<T>");
  Assign(first, last);
}

// After
template <typename IterType, typename = std::enable_if_t<is_valid_iterator_v<T, IterType>>>
Array(IterType first, IterType last) {
  Assign(first, last);
}

That said, if this is the only instance of this issue, it probably is easier to change from Array{...} to Array<ObjectRef>{...}.

Lunderberg avatar Apr 18 '23 15:04 Lunderberg

@Lunderberg Thanks for your efforts. Indeed, only one instance of this issue.

After changing Array{...} to Array<ObjectRef>{...}. and changing the source code you provided above, build sucessfully.

jikechao avatar Apr 20 '23 06:04 jikechao

@tvm-bot rerun

Lunderberg avatar May 09 '23 20:05 Lunderberg

Trying to clear out some of my (very) old PRs. This PR is now rebased onto main including the fix in this comment. If it passes CI, then it's ready to merge.

Lunderberg avatar May 23 '24 18:05 Lunderberg