ABataev added a comment. 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. 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. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
