Deprecate ParamMap (#7121)

[steven-johnson]

This PR deprecates ParamMap for Halide 16, with the plan of removing it entirely for Halide 17; it was added to provide a threadsafe way to provide parameteres to the JIT, but compile_to_callable() now does this in a much less intrusive way.

[abadams]

This is a pretty scary claim. Can we be more precise? Does this test actually share Halide objects across multiple threads? If not, it should be safe. I think the issue with simd_op_check was that it did.

[abadams]

Looking into history, the thread-unsafety we had was this: https://github.com/halide/Halide/pull/3914/files

Sharing objects between multiple threads and calling non-const methods on them breaks. But that's true of most libraries.

[soufianekhiat]

To my understanding, currently without ParamMap, we can't have set parameters programmatically.
And is ArgvStorage is private, we can't do "fill_slot" by hand. removing ParamMap will remove this feature right?

[abadams]

Can you post an example of how you're using ParamMap? We should be able to enable equivalent functionality somehow.

[soufianekhiat]

Something like that:

Func f = GetFunc();
Pipeline p( f );

Buffer<f32> oHInputsA( ... );
Buffer<f32> oHKernel( ... );
f32 fParam = ...;

Buffer<f32> aHOutputs( ... );

ParamMap params;
///
params.set(src, oHInputsA);
params.set(kernel, oHKernel);
params.set(param0, 5);
params.set(param1, fValue);
///
p.realize(aHOutputs, t, params);

Where what's between '///' is generated via a data driven system.

[abadams]

Is this machine-generated source code, or does the bit between the /// markers stand in for a for loop over parameters or something? If it's machine-generated source, surely it's easy enough to just generate code that uses param.set(oHInputsA) instead. If it's not, consider using Callable::call_argv_fast

[soufianekhiat]

More a loop.
I'm just surprise about the input.
const void *const * argv.
That should be an array to pointers?

Something like that?

void const* pData[5];
pData[0] = &oHInputsA;
pData[1] = &oHKernel;
int five = 5;
pData[2] = &five;
pData[3] = &fValue;
pData[4] = &aHOutputs;

std::vector< Argument > args = { src, kernel, param0, param1 };
Callable c = p.compile_to_callable( t, args );

c( 5, &pData[0] );

[abadams]

Basically yeah, but the first argument needs to be a Halide::JITUserContext **. I think it goes something like this:

void const* pData[6];
JITUserContext ctx;
JITUserContext *ctr_ptr = &ctx;
pData[0] = &ctx_ptr;
pData[1] = oHInputsA.raw_buffer();
pData[2] = oHKernel.raw_buffer();
int five = 5;
pData[3] = &five;
pData[4] = &fValue;
pData[5] = aHOutputs.raw_buffer();

std::vector< Argument > args = { src, kernel, param0, param1 };
Callable c = p.compile_to_callable( t, args );

c.call_argv_fast( 6, &pData[0] );

[steven-johnson]

Can you expand a bit on why using Callable isn't an acceptable replacement?

[steven-johnson]

Is this ready to land, or do we have more concerns about breaking existing users?

[soufianekhiat]

Can you expand a bit on why using Callable isn't an acceptable replacement?

I wasn't aware of 'call_argv_fast' which seem a good replacement, just curious is that still work for buffer in GPU, do we still have a way to have set_host_dirty etc? Because for GPU clearly it's not raw_buffer we need.

[steven-johnson]

Callable should work just as well as realize() for GPU buffers -- it's the same underlying mechanism being used.

Both halide_buffer_t and Halide::Runtime::Buffer() have set_host_dirty() etc methods.

[soufianekhiat]

Thanks

[soufianekhiat]

Another comment,
By trying to convert my code I notice some caveat, a Callable cannot be stored in a structure.
As we don't have default constructor or copy/affectation.

I mean in a larger system, we would like to store a Callable for later usage.

struct SingleFunc
{
    Pipeline  oPipeline;
    Callable  oCallable;
    Callable* pCallable;
};

It's not possible to have:
oSingleFunc.oCallable = oSingleFunc.oPipeline.compile_to_callable( oSingleFunc.aArgs, oSingleFunc.oTarget );
Or that following line compile but I wasn't able to find the code for the copy constructor, that should be the default one generated by the compiler:
oSingleFunc.pCallable = new Callable( oSingleFunc.oPipeline.compile_to_callable( aArgs, oTarget ) );.

Do you have any comment on that?

[steven-johnson]

Another comment, By trying to convert my code I notice some caveat, a Callable cannot be stored in a structure. As we don't have default constructor or copy/affectation.

It has the same storage requirements as every other piece of Halide IR -- e.g., Expr, Func, Param, etc, in that they are all just wrappers over a hidden, refcounted 'contents'.

I mean in a larger system, we would like to store a Callable for later usage.

struct SingleFunc
{
    Pipeline  oPipeline;
    Callable  oCallable;
    Callable* pCallable;
};

It's not possible to have: oSingleFunc.oCallable = oSingleFunc.oPipeline.compile_to_callable( oSingleFunc.aArgs, oSingleFunc.oTarget );

If you can store a Pipeline there, you should be able to store a Callable. What is the specific error you are getting?

Also: why do you want/need to store a Callable *?

[soufianekhiat]

Callable have a default constructor as private:
So:

struct SingleFunc
{
    Callable  oCallable; // So this do not compile
};

So:
'Halide::Callable::Callable': cannot access private member declared in class 'Halide::Callable'

[abadams]

It's possible to use a struct like that with types that don't have public default constructors by not default-constructing the struct. But that's sort of a pain. If we want Callable to act like other front-end types, we should make it nullable, by making the default constructor public and add a .defined() method.

[soufianekhiat]

My struct contains other stuff I need my default constructor.
For now I'm stuck with ParamMap.

[steven-johnson]

making the default constructor public and add a .defined() method

That's what we'll do, PR on the way

[soufianekhiat]

Thanks a lot

[steven-johnson]

#7380

[steven-johnson]

@soufianekhiat -- the fixes for Callable have landed. Please check them out and let us know if this will make Callable an acceptable replacement for ParamMap -- I don't want to land this until I hear back :-)

[soufianekhiat]

I'm still testing, on 4a80251.
I have exception on compile_to_callable with a simple kernel:

// Before

ImageParam p( f32, 0, "input" );
Func f{ "naive" };
f() = p();

Pipeline p( f );
p.compile_jit( oTarget );
oHOutput, oTarget, oParams
ParamMap oParams;
f32 five = 5.0f;
Buffer<> buff = Buffer<>::make_scalar( &five, "p_input" );
oParams.set( p, buff );
Buffer<> output = Buffer<>::make_scalar( Float( 32 ), "output" );
p.realize( output, oTarget, oParams );

// Now:

Pipeline p( f );
Callable oCallable = p.compile_to_callable( { p }, oTarget );

I got an exception, with exception & HL_DEBUG_CODEGEN=1:

Lowering Parallel Tasks...
User error triggered at F:\git\Halide\src\Lower.cpp:527
Returned from fiber.
Fiber threw exception. Rethrowing...

Still debugging my code

[steven-johnson]

Thank you for looking into this, I really appreciate it. Please keep me up to date.

[steven-johnson]

Callable oCallable = p.compile_to_callable( { p }, oTarget );

The arguments to compile_to_callable are the same as for compile_to_file, etc: the first argument is a list of the Param and/or ImageParams that are the arguments to the function. In your example, I see a p that is an ImageParam (which would be the right thing to use), but also a p that is the Pipeline itself (which would be very wrong and should fail at compile-time). Without seeing the entire context of your change I can't tell for sure which is in scope, but it sure looks like the latter.

[steven-johnson]

Monday review ping -- I'm going to land this if I don't hear back from @soufianekhiat soon

[soufianekhiat]

I'm still working on convert my the code.
Currently I have issue to integrating on my system.
The code snippet is working well.
But my setup is as follow with multiple thread.

1. Compiler + auto-schedule thread
2. Worker group for executions

Currently I'm crashing on random memory, I'll rebuild Halide on debug to track the issue.

If it's urgent you can merge it until it's not removed, and we can continue on a discussion/issues somewhere else.

[steven-johnson]

No, it's not urgent, thanks for your response. It would be better to wait and be sure that you have a workable solution.

[steven-johnson]

(Just back from vacation -- wanted to check in with @soufianekhiat to see if there's any update. Still no rush.)

[steven-johnson]

Monday Morning Review Ping.

[soufianekhiat]

Hi,
I'm back from sick leave, I'm back now on this today.

[soufianekhiat]

Finally, I was able to convert my system by using call_argv_fast.
Everything look good to me know. Thanks for your time.