<div dir="ltr"><div dir="ltr">Thanks for the comments Johannes.<div><div><br></div><div>Anyone has comments?</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 3, 2019 at 5:47 PM Doerfert, Johannes <<a href="mailto:jdoerfert@anl.gov">jdoerfert@anl.gov</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">(I tried to collect the cross-posts from the cfe and openmp list here,<br>
unsure if I got the latest ones though. No more cross posts and/or<br>
dropping a list please.)<br>
<br>
> Ravi's email to cfe-dev<br>
><br>
> Everytime we want to pass something new to the libomptarget runtime we<br>
> would need to add a new interface. Why not pass a pointer to a<br>
> structure, 1st field says how many valid fields this structure<br>
> contains and populate the fields with info or null. Run time can<br>
> check if the field is null or not and take appropriate action.<br>
> <br>
> Struct {<br>
> Int Num_fields 2<br>
> Int * mapper;<br>
> Int * nowait_object<br>
> }<br>
> <br>
> To support async (aka nowait) we need an object to call back to the<br>
> libomp library. This object can be passed in this struct.<br>
<br>
Initially, I thought this was a similarly good design. While fairly<br>
future prove, I have some problems I want to mention. First, however,<br>
I'd like to point out that new API functions alone are not really<br>
problematic though, especially if they are just wrappers around the<br>
newest version.<br>
<br>
Drawbacks of a "version struct":<br>
- It makes the API backwards compatible, at least until runtime. I<br>
mean, when you recompile a component of your application with a<br>
(maybe newer) compiler it is unclear what version the rest of the<br>
application, and the used structs, are using. If you assume the<br>
struct passed around is at least of the current version, it will<br>
break if it is not, silently at runtime. If you don't assume<br>
anything, you need to emit dynamic checks and versioning when you<br>
want to modify the code. The same situation with the "new API" model<br>
will cause a link error if the runtime you link into is not as new<br>
as the API calls you used. This allows you to rewrite the code based<br>
on the API call semantic you know without versioning and fear of<br>
silent miscompilation. I know this explanation is very abstract, let<br>
me know if I should try to make it more concrete.<br></blockquote><div><br></div><div>I agree. In this case, the runtime will need to check 'Num_fields' to make sure it only uses what the struct contains. But I have to say Ravi's scheme is quite extendable.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- We cannot remove arguments in a reasonable way. Please correct me if<br>
I'm wrong here.<br></blockquote><div><br></div><div>I agree. I think you can only add arguments in this case.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
On 10/01, Jon Chesterfield via cfe-dev wrote:<br>
> ><br>
> ><br>
> > I would like to bring your attention to the choice of 2 proposals for the<br>
> > declare mapper runtime interface:<br>
> ><br>
> > 1. The current design which creates new runtime functions for declare<br>
> > mappers. For example, right now we have `__tgt_target_teams(...)` which<br>
> > corresponds to the runtime interface for `omp target teams`. Now we add<br>
> > `__tgt_target_teams_mapper(..., void **mappers)` to replace it.<br>
> ><br>
> <br>
> New feature, new library call seems reasonable. The current interface can<br>
> presumably be reimplemented as a call to (the internals of) the proposed<br>
> interface.<br>
<br>
Yes, I mentioned that as well:<br>
old(args) = new(args, nullptr)<br>
<br>
> > 2. Introduce a function `__tgt_push_mappers`, which should be called before<br>
> > every target function call (e.g., `__tgt_target_teams`) to pass the mapper<br>
> > argument for that function. The call of `__tgt_push_mappers` is implicitly<br>
> > bonded with the actual target call.<br>
> ><br>
> <br>
> This seems error prone to use and likely to degrade performance.<br>
> <br>
> Choice 1 seems more appealing to me.<br>
<br>
So far, most people seem to prefer choice 1. We should go with that one<br>
if there are no complaints soon.<br>
<br>
<br>
Cheers,<br>
Johannes<br>
</blockquote></div></div>