Page MenuHomePhabricator

Rework ExecutionEngine::invoke() to make it more friendly to use from C++
ClosedPublic

Authored by mehdi_amini on Feb 3 2021, 10:32 AM.

Details

Summary

This new invoke will pack a list of argument before calling the
invokePacked method. It accepts returned value as output argument
wrapped in ExecutionEngine::Result<T>, and delegate the packing of
arguments to a trait to allow for customization for some types.

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 3 2021, 10:32 AM
mehdi_amini requested review of this revision.Feb 3 2021, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 10:32 AM
rriddle added inline comments.Feb 3 2021, 12:01 PM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
110
118

Is this class just for readability?

aartbik added inline comments.Feb 3 2021, 12:20 PM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
149

can you indent this a bit more right for readability?

155

every argument or all arguments

mlir/unittests/ExecutionEngine/Invoke.cpp
2

copy and paste left over

mehdi_amini added inline comments.Feb 3 2021, 1:23 PM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
118

No it is actually load bearing, I use it in the trait specialization. This is tricky to describe in a comment though, this is due to our ABI where we take every operand by pointers:

float foo(float, float);

is turned into this interface:

void foo_ifcase(**float) {
  float *arg0 = float[0];
  float *arg1 = float[1];
  float *arg2 = float[2];
  *arg2 = foo(*arg0, *arg1);
}

Now from C++ we would want to invoke the jit like this:

float result;
float input = 1.0f
jit->invoke("foo", 1.f, input, result);

To be able to accept the literal argument, we can't just take a non-const reference so I take the arguments by values instead. But we want a reference to the result, this wrapper achieves this here:

jit->invoke("foo", 1.f, input, ExecutionEngine::Result<float>(result));

That way we carry the address of result into the invoke.

An alternative would be to not allow literal and other const objects on the interface I think.

mehdi_amini marked 4 inline comments as done.

Address comments

mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
149

Ah I had it on a single line and clang-format wrapped it. Fixed!

155

My English is terrible... Thanks!

Remove spurious CMake change

aartbik added inline comments.Feb 3 2021, 5:11 PM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
155

Better than mine, but I don't see this changed? Did you upload?

ftynse accepted this revision.Feb 4 2021, 1:18 AM
ftynse added inline comments.
mlir/unittests/ExecutionEngine/Invoke.cpp
40–43

I don't think we need any of these

92

Would it make sense to have a function

template <typename T>
Result<T> result(T &t) { return Result<T>(t); }

to trigger type deduction here as ExecutionEngine::result(result), until we get deduction guides in C++17?

This revision is now accepted and ready to land.Feb 4 2021, 1:18 AM
mehdi_amini added inline comments.Feb 4 2021, 9:14 AM
mlir/unittests/ExecutionEngine/Invoke.cpp
40–43

Right, I locally have further tests that I was playing with involving linalg/vector, but I'll remove these for now!

92

Ah excellent! I don't know why I didn't think of it :)

aartbik added inline comments.Feb 4 2021, 9:17 AM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
157

friendly ping on this

mehdi_amini added inline comments.Feb 4 2021, 9:34 AM
mlir/include/mlir/ExecutionEngine/ExecutionEngine.h
155

Indeed I amended it in the wrong commit in my chain...

mehdi_amini marked 5 inline comments as done.Feb 5 2021, 5:31 PM

Address comments

This revision was landed with ongoing or failed builds.Feb 5 2021, 5:33 PM
This revision was automatically updated to reflect the committed changes.