[FFI] Allow IntImm arguments to PackedFunc with int parameter

[Lunderberg]

TVM containers, such as tvm::runtime::Array, require the contained objects to inherit from ObjectRef. As a result, the wrapper types IntImm, FloatImm, and StringImm are often used to allow native types in the TVM containers. Conversions into these wrapper type may be required when using a container, and may be performed automatically when passing an object across the FFI. By also providing conversion to an unwrapped type, these automatic conversions are transparent become transparent to users.

[Aleksei-grovety]

Thanks @Lunderberg! Looks good, just some comments.

[Aleksei-grovety]

Shouldn't there be static_cast<double>, as in line 588?

[Lunderberg]

Good catch. It isn't strictly necessary in either case, due to C++'s implicit conversion of numeric types, but would be good to call attention to it. Updated to use a static_cast<double> here.

[Aleksei-grovety]

Trailing whitespace, also in lines 550, 552.

[Lunderberg]

Hmm, I'm not seeing the trailing whitespace in the commit, either using git show --word-diff-regex="[ ]+|[^ ]+", setting my editor to show whitespace, or in the github diff.

[Aleksei-grovety]

Sorry, I used the wrong definition, there is a double space before "As"

[Lunderberg]

Ah, I see. This is intentional, but is more of a stylistic difference than anything else. Traditionally, (accidental diversion into typography) a space after a sentence would use an em space (the same with as a capital M) after a sentence, wider than the spacing between words. This isn't possible in monospaced fonts, so typewriters would emulate the em space by using two spaces. Exactly which is better has since become the topic of a great many flame wars.

I was curious, and it looks like double-spacing after a sentence is about twice as common as single-spacing in the TVM repo, with the Apache copyright header being the most prominent example.

# Count the number of sentences followed by one space
$ find . \
  \( -path ./3rdparty -o -path "./build*" \) -prune -o \
  \( -name "*.cc" -o -name "*.h" \) \
  -exec grep '[A-Za-z]\{2\}\. [A-Za-z]' {} /dev/null \; \
    | wc --lines
3958

# Count the number of sentences followed by two spaces
$ find . \
  \( -path ./3rdparty -o -path "./build*" \) -prune -o \
  \( -name "*.cc" -o -name "*.h" \) \
  -exec grep '[A-Za-z]\{2\}\.  [A-Za-z]' {} /dev/null \; \
    | wc --lines
7666

[Aleksei-grovety]

Trailing whitespace.

[tqchen]

Given the implementation complexity i feel it would be helpful to not support this implicit behavior for now. This is because IntImm are used in other purposes(e.g. that comes with dtype).

This may start to make sense once we start to introduce a BoxInt that do not contain the dtype field and simply serve as a dtype function.

I assume downstream packedfunc will still be able to handle the function correctly by either expecting the object or non-object variant

[Lunderberg]

This arose when writing unit tests for access of a relax tuple. I expected the following unit test to pass on unity HEAD.

import tvm.testing
from tvm.script import relax as R

exec_mode = tvm.testing.parameter("bytecode", "compiled")

def test_vm_tuple_get_item(exec_mode):
    @R.function(private=True)
    def func(arg: R.Tuple([R.Prim("int64"), R.Prim("float32")])):
        return arg[0]

    mod = tvm.IRModule({"main": func})

    target = tvm.target.Target("llvm", host="llvm")
    ex = tvm.relax.build(mod, target, exec_mode=exec_mode)
    vm = tvm.relax.VirtualMachine(ex, tvm.cpu())

    res = vm["main"]((17, 42.5))
    assert res == 17

The error occurs with the following steps:

1. When passed an argument, python tuples are converted to tvm.ir.Array.
2. In order to use tvm.ir.Array, the arguments must be converted to ObjectRef subclasses. This converts (17, 42.5) into tvm.ir.Array([ T.int64(17), T.float32(42.5) ]).
3. The tuple is accessed with a relax VM builtin, and T.int64(17) is stored in a relax VM register.
4. The T.int64(17) is type-checked as a R.prim('int64'), which uses TVMPODValue_::operator int64_t.
5. The call to operator int64_t fails, as it is an IntImm, and uses TVMArgTypeCode::kTVMObjectHandle and not TVMArgTypeCode::kTVMArgInt.

Any of these steps could be broken in order to avoid the error. Fundamentally, the error occurs because we apply an automatic conversion (int to IntImm), but do not automatically apply the reverse conversion (IntImm to int). I believe that this PR, which changes step (5), is the best resolution. Avoiding the error by updating any of the other steps has drawbacks as described below.

1. Change the FFI conversion of python tuples to a runtime-only type, rather than tvm.ir.Array. This would cause breakage for any FFI call that uses Array at compile-time.
2. Change the internal type of tvm.ir.Array from ObjectRef to TVMArgValue. This would allow FFI conversions from python tuples to Array, but would break the usage of those Array objects.
3. Change the return of the tuple_getitem builtin to convert from IntImm to int when providing a TVMRetValue. Possible, but would drop the dtype information that may be required for later type-checks.
4. Change the type-checking of R.Prim in the VM to check for IntImm. This would avoid the error in this case, but this function is used for type-checking before calling external PackedFunc instances. Those calls would throw the same error at a later point.
5. Change the operator int64_t implementation to check for an IntImm. That is the change implemented in this PR, and handles any such conversion required.

Given the implementation complexity

Can you explain what you mean here? This doesn't seem any more complex than the argument handling that we already do for FFI compatibility purposes.

[tqchen]

I understand the error scenario you described, and also agree this is an issue when we start to involve tuple of prim values.

I just would like to bring up the point about path to support these cases. One current issue is that IntImm and FloatImm are compile time construct that are not defined as partof the runtime (because they are coupled as Expr).

As a result, generalizing IntImm/FloatImm implementation won't allow us to support the case where only tvm_runtime is available, which is a common case to support.

A better approach would be to enable boxed BoxInt and Float that are decoupled from IntImm and FloatImm, which can exist in the runtime, and migrate the existing usage of IntImm to those runtime boxed cases.

Given tuple of two prim values are less commonly used and we only recently supported the prim values. I think it is better for us to not support the case for now, so we can later go for a longer term solution that separates out dedicated runtime boxed values, in which case automatic conversion would makes sense and there won't be limitations that it won't work in a runtime onlys etting. In the meantime, we first report error for cases that involves tuple of prim values, and expand our scope of support once the boxed version of values land(which enables generic solution for runtime only cases as well)

[Lunderberg]

A better approach would be to enable boxed BoxInt and Float that are decoupled from IntImm and FloatImm, which can exist in the runtime, and migrate the existing usage of IntImm to those runtime boxed cases.

I like the idea of having the separate runtime-only types, as distinct from the compile-time representations. I was concerned that that would be a larger impact change, but I agree with that as the long-term approach.

For now, closing this PR to avoid any premature merging.

Given tuple of two prim values are less commonly used and we only recently supported the prim values.

For my understanding, I thought that PrimValue had always been present, back to the initial AST definition in #13901. Use of a PrimValue to define a symbolic variable is relatively recent, but the failure mode doesn't depend on that functionality.

[Lunderberg]

This PR has been superseded by #16183, which implements boxed primitives that can be used as part of libtvm_runtime.so, along with the type conversions required for backwards compatibility. This includes the conversion of a boxed primitive to an unboxed primitive, which is the functionality required for relax tuples containing primitives.