[RUST][FRONTEND] macro for constructing the builder

[ehsanmok]

The proposed tvm_call! doesn't not accept set_output. Expand the macro with perhaps some related syntax extension. TODO #2292

[tqchen]

to avoid creating too many issues, please merge most of them into a single rust FRONTEND working item issue and only open new issue when we think we can act on it soon(say can be closed in two weeks)

[ehsanmok]

Noted! Given the upcoming holidays, might be qualified in the timeline.

[ehsanmok]

@tqchen @nhynes I decided to keep the ball rolling before the holidays. Regarding the macro, currently tvm_call! is good enough. My suggestion is that we can generalize it to something like

tvm_call!(func: [func_identifier], arg: [stack of args], out: [at most one output])

and to ensure the output to have at most one value, we can use !#[feature(macro_at_most_one_rep)] through ?, which we won't need in 2018 edition actually.

In my opinion this is rather an icing-on-the-cake macro. What do you think?

[nhynes]

what you're describing seems almost exactly what runtime::{PackedFunc, call_packed} do. Is there something that tvm_call must do that call_packed does not?

[nhynes]

Actually, on second reading. I'm really unconvinced that all of this Builder stuff is necessary since you can always just cast the TVM function to an actual Box<Fn> and call it directly. Would you mind explaining the motivations for using the Builder pattern instead of using functions directly? I think this is worth more careful consideration given the amount of complexity it introduces.

[ehsanmok]

@nhynes Honestly, I don't see any added complexity.

1. Builder pattern is more common in Rust than in any other languages. I like to think that making the components clear and out of magic is the proper way in general and in particular for our case. For example, during dev, I wanted to investigate the args passed in the builder for debugging purposes. Debugging macros in Rust is not graceful even with log_syntax or trace_macros.

2. I developed the frontend with compatibility of other runtime frontends in mind (such as java) to make things more coherent than runtime when added recently. Builder was also used there.

3. Builder also covers your syslib.get_function("default_function") when interacting with already built modules as well as invoking the functions.

4. tvm_call is pretty similar to call_packed and I'm for the name change as I suggested in the PR comments before, but right now, tvm_call doesn't use set_output as in

function::Builder::from(&mut fadd)
            .arg(&arr)
            .arg(&arr)
            .set_output(&mut ret)
            .invoke()
            .unwrap();

which I thought this issue is about and making it general can make things more clearer by something like

tvm_call!(fadd; args: &arr, &arr; out: &mut ret)?;

5. I think packed function fn(&[TVMArgValue]) -> Result<TVMRetValue> is more Rusty than Box<Fn(&[TVMArgValue]) -> TVMRetValue + Send + Sync>. In fact, fn function ptr is Fn + Send + Sync + Pointer etc.

Finally, frontend needs to be able to register global functions and the whole mechanism fits things together in register_global_func!

[nhynes]

Okay, then let's modify PackedFunc to return Result<TvmRetValue>. Having two ways to construct and call PackedFunc isn't bad so how about we do both macro and builder but simplify the interior of builder to use PackedFunc.

[ehsanmok]

Sure! since this issue is addressing your first proposition, shall we move on and make a succinct plan after frontend is merged in next iteration? note that there's definitely room for improvements from both side and we're just releasing v0.1 which is somewhat far from TVM v0.5 (if matching it is a concern).

[nhynes]

Sure Builder can be improved once we merge everything in. We're going to need to update TVMRetValue now, though, since that's going to become a common dependency pre-merge.