ABataev added a comment. In D71179#1778512 <https://reviews.llvm.org/D71179#1778512>, @hfinkel wrote:
> 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. I agree that reusing is a good idea but not in this case. I already wrote that Johanmes reuses just a single feature of the multiversioning - handling of the multiple definitons for the same function. Nothing else. Everything else is a new functionality to support declare variant stuff. > > > 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. Current solution allows to do everything correctly and requires just a small rework. The actual selection of the fumction can (and must be done) at the codegen. There is no benefits inchoosing correct variant functiin in sema. Moreover, it leads to breaking of the AST in the way that the original function call is replaced by a new function call in AST. And dump/printing works differently than the user expected. L > > >> 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. There are no advantages at all. This solution has absolutely the same power as the existing one. And I see not a single reason why a function resolution should happen in sema. Moreover, there are real problems with the proposed solution with AST. 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
