[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

Tue Dec 10 15:57:55 PST 2019

hfinkel added a comment.

In D71179#1776761 <https://reviews.llvm.org/D71179#1776761>, @ABataev wrote:

> In D71179#1776528 <https://reviews.llvm.org/D71179#1776528>, @hfinkel wrote:
>
> > In D71179#1776491 <https://reviews.llvm.org/D71179#1776491>, @ABataev wrote:
> >
> > > In D71179#1776487 <https://reviews.llvm.org/D71179#1776487>, @hfinkel wrote:
> > >
> > > > In D71179#1776467 <https://reviews.llvm.org/D71179#1776467>, @ABataev wrote:
> > > >
> > > > > In D71179#1776457 <https://reviews.llvm.org/D71179#1776457>, @jdoerfert wrote:
> > > > >
> > > > > > > You're doing absolutely the same thing as the original declare variant implementation.
> > > > > >
> > > > > > I don't think so but if you do why do you oppose this approach?
> > > > > >
> > > > > > > And I don't think it would be correct to add them as multiversiin variants to the original function.
> > > > > >
> > > > > > Why wouldn't it be correct to pick the version through the overload resolution instead of the code generation?
> > > > > >  How this could work is already described in the TODO (CodeGenModule.cpp):
> > > > > >
> > > > > >   // TODO: We should introduce function aliases for `omp declare variant`
> > > > > >   //       directives such that we can treat them through the same overload
> > > > > >   //       resolution scheme (via multi versioning) as `omp begin declare
> > > > > >   //       variant` functions. For an `omp declare variant(VARIANT) ...`
> > > > > >   //       that is attached to a BASE function we would create a global alias
> > > > > >   //       VARIANT = BASE which will participate in the multi version overload
> > > > > >   //       resolution. If picked, here is no need to emit them explicitly.
> > > > > >   
> > > > > >
> > > > > >
> > > > > >
> > > > > >  ---
> > > > > >
> > > > > > I still haven't understood why we cannot/should not reuse the existing multi-version support and instead duplicate the logic in some custom scheme.
> > > > > >  We have this patch that shows how we can reuse the logic in Clang. It works on a per-call basis, so it will work for all context selector (incl. construct).
> > > > > >  If you think there is something conceptually not working, I'd like to hear about it. However, just saying "it wouldn't be correct" is not sufficient. You need to provide details about the situation, what you think would not work, and why.
> > > > >
> > > > >
> > > > > I explayned already: declare variant cannot be represented as mutiversion functiin, for example.
> > > >
> > > >
> > > > @ABataev, can you please elaborate? It's not obvious to me that we cannot handle the existing declare variant with the same scheme (as @jdoerfert highlighted above). In general, I believe it's preferable to have one generic scheme and use it to handle all cases as opposed to continuing to use a more-limited scheme in addition to the generic scheme.
> > >
> > >
> > > Eaine already. Current version of declare variant cannot be represented as multiversiin functions, because it is not. We have a function that is the alias to other functions with different names. They just are not multiversion functions by definition.
> >
> >
> > I understand that they have different names. I don't see why we that means that they can't be added to the overload set as multi-version candidates if we add logic which does exactly that.
>
>
>

@jdoerfert posted a prototype implementation in D71241 <https://reviews.llvm.org/D71241>, so we don't need to just have a theoretical discussion, but I'd like to address a high-level issue here:

> Because this is exactly what I said- you want to reuse the exiwting solution for completely different purpose just because you want to you reuse though even semantically it has nothing to do with multiversioning.

This kind of comment really isn't appropriate. We're all experienced developers here, and no one is proposing to reuse code in an inappropriate manner "just because" or for any other reason. I ask you to reconsider your reasoning here for two reasons:

1. "Reus[ing] the existing solution for a completely different purpose", which I'll classify as structural code reuse, is not necessarily bad. Structural code reuse, where you reuse code with a similar structure, but different purpose, from what you need, is often a useful impetus for the creation of new abstractions. The trade off relevant here, in my experience, is against future structural divergence. In the future, is it likely that the abstraction will break down because the different purposes will tend to require the code structure to change in the future in divergent ways? If so, that can be a good argument against code reuse.
2. Your statement of differing purpose, that declare variant has "nothing to do with multiversioning", it not obviously true. Declare variant, as the spec says, "declares a specialized variant of a base function and specifies the context in which that specialized variant is used." multiversioning, according to the GCC docs, makes it so that "you may specify multiple versions of a function, where each function is specialized for a specific target feature. At runtime, the appropriate version of the function is automatically executed depending on the characteristics of the execution platform." These two concepts do share some conceptual relationship.

It certainly seems fair to say that the AST representation desired for a call with runtime dispatch might be sufficiently different from a call resolved at compile time to make the code reuse inadvisable. However, the requirements of which I'm aware are: the representation should be unambiguous and faithful to the source and language structure, and also, the statically-resolvable callee should be referenced by the call site in the AST. As can be seen in this patch and the associated tests, both of these requirements are satisfied.

> And I think it is bad idead to break the semantics of the existing solution. It requires some addition changes like merging of different functiins with different names. And here I want to ask - why do you think it is better than my proposal to reuse the codegen for the already implemented declare variant stuff for the OpenMP  multiversioned functions? It really requires less work, bdcause you just need to add a loop over all varinants and call `tryEmit...` function.

When you say "semantics of the existing solution", do you mean the extent to which it satisfies the standard, non-standard user-visible behaviors of the existing implementation, or the internal structure of the implementation? It's certainly not a bad idea to change, or even replace, the current implementation if the result is better in some way (e.g., more general, supports more features, conceptually cleaner). The proposed solution seems to have a number of advantages over the current solution in codegen, and in addition, naturally handles the new features that we would like to support. Generally, resolution of static calls is something that should happen in Sema, not in CodeGen, in part so that static analysis tools can easily understand the call semantics. This new approach naturally provides this implementation property.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71179/new/

https://reviews.llvm.org/D71179