Default mapper: mark `default_make_instance` and `default_create_custom_instances` as virtual

[mariodirenzo]

It would be useful to be able to overwrite the two methods mentioned in the title in mappers that inherit from the default mapper.

@elliottslaughter, can you add this issue to #1032 ?

[lightsighter]

Why do you want to do that? Really only the default_policy_* methods are designed to be overridden since they are policies that you might want to change. If you find yourself wanting to override the actual mechanism functions like default_make_instance (which does what it says without any policy considerations) then that probably suggests you want to be overriding the enclosing mapper interface function instead of messing with the mechanism functions of the default mapper which need to have predictable semantics for all mappers that call them.

[mariodirenzo]

The current implementation of default_make_instance forces every region requirement with reduction privileges to create a new reduction instance without checking if there are existing compatible instances (see https://gitlab.com/StanfordLegion/legion/-/blob/master/runtime/mappers/default_mapper.cc?ref_type=heads#L2437).

In my case, I have three tasks, say tasks A, B, and C, launched in succession and with reduction privileges on the same field and region. Tasks A and C apply the same layout constraints on the region requirement with reduction privileges. Task B has different layout constraints. To my understanding, the current implementation of the mapper will always force tasks A, B, and C to have different reduction instances. I would like tasks A and C to share the same reduction instance and B to have its own. I do not think that I can achieve this behavior by just overriding the default_policy_* methods

[lightsighter]

The current implementation of default_make_instance forces every region requirement with reduction privileges to create a new reduction instance without checking if there are existing compatible instances

Isn't that true of all instances though? If you're calling default_make_instance then you're making an instance no matter what unless you set force_new=false. We should support setting force_new=false for reduction instances which will call find_or_create_physical_instance, but that shouldn't necessitate overriding default_make_instance.

[mariodirenzo]

Isn't that true of all instances though? If you're calling default_make_instance then you're making an instance no matter what unless you set force_new=false

That's right. This pattern is particularly critical for the performance of one section of the code and overriding default_make_instance would have been the quickest fix without requiring significant effort from the legion team. I agree that the better fix is allowing the default mapper to reuse reduction instances, as discussed in #1703.

[lightsighter]

Ok, I think in general for things like this that fixing the default mapper is probably the right answer rather than allowing default_* functions to be overridden. I think that is likely to cause more problems than it fixes as lots of stuff in the default mapper depends on the semantics of those functions if users change those semantics out from under us then lots of things might be prone to breaking. We're always open to receiving pull requests on the default mapper and they will receive far less scrutiny than stuff that goes into the runtime itself.