ABataev added a comment. In D71241#1782812 <https://reviews.llvm.org/D71241#1782812>, @hfinkel wrote:
> In D71241#1782779 <https://reviews.llvm.org/D71241#1782779>, @ABataev wrote: > > > In D71241#1782742 <https://reviews.llvm.org/D71241#1782742>, @hfinkel wrote: > > > > > In D71241#1782703 <https://reviews.llvm.org/D71241#1782703>, @ABataev > > > wrote: > > > > > > > > > > > > > > > > ... > > > > > > >>>>>> > > > >>>>>> > > > >>>>>>> Given we have two implementations, each at different points in > > > >>>>>>> the pipeline, it might be constructive to each write down why you > > > >>>>>>> each choose said point. I suspect the rationale is hidden by the > > > >>>>>>> implementation details. > > > >>>>> > > > >>>>> Actually, early resolution will break tbe tools, not help them. It > > > >>>>> will definitely break clangd, for example. The user will try to > > > >>>>> navigate to originally called function `base` and instead he will > > > >>>>> be redirected to the function `hst`. > > > >>>> > > > >>>> Can you please be more specific? Please explain why the user would > > > >>>> consider this incorrect behavior. If the point of the tool is to > > > >>>> allow the user to navigate to the function actually being called, > > > >>>> then navigating to `base` seems incorrect if that's not the function > > > >>>> being called. This is just like any other kind of overload > > > >>>> resolution - the user likely wants to navigate to the function being > > > >>>> called. > > > >>>> > > > >>>> Now the user might want an OpenMP-aware tool that understands > > > >>>> differences between host and accelerator behavior, how that affects > > > >>>> which functions are called, etc. The user might want this for > > > >>>> host/device overloads in CUDA too, but this is really an orthogonal > > > >>>> concern. > > > >>> > > > >>> You wrote the code. You called a function in the expression. Now you > > > >>> want to navivate to this function. Clicked on it and instead of the > > > >>> called `base` you are redirected to `hst` because AST has the link to > > > >>> `hst` functiin inthe expression instead of the `base`. > > > >> > > > >> Sure, but it has that link because that `hst` function is actually the > > > >> function being called (assuming that the clangd-using-tool is > > > >> configured to interpret the code as if compiling for the host). When I > > > >> click on a function call in a source file, I want to navigate to the > > > >> function actually being called. I certainly understand that the > > > >> function being called now depends on compilation context, and the tool > > > >> in our example is only providing the resolution in one context, but at > > > >> least it provides one valid answer. An OpenMP-aware tool could > > > >> navigate to the base function (we do need to preserve information to > > > >> make this possible). This is just like dealing with some host/device > > > >> functions in CUDA (where there are separate overloads) - if you click > > > >> on the function in such a tool you'll probably navigate to the host > > > >> variant of the function (even if, in some other context, the device > > > >> overload might be called). > > > >> > > > >> Again, I see this as exactly analogous to overload resolution, or as > > > >> another example, when calling a function template with > > > >> specializations. When using such a tool, my experience is that users > > > >> want to click on the function and navigate to the function actually > > > >> being called. If, for example, I have a function template with > > > >> specializations, if the specialized one is being called, I should > > > >> navigate to the specialization being called (not the base function > > > >> template). > > > > > > > > You wrote wrong context matcher. You has a bug in the base function, > > > > which should be called by default everu sw here but the host, and want > > > > to fix it. Etc. > > > > > > I understand, but this is a generic problem. Same with host/device > > > overloads in CUDA. Your tool only gets one compilation context, and thus > > > only one callee. In addition, FWIW, having a base version called > > > everywhere except on the host seems like an uncommon use case. Normally, > > > the base version *is* the version called on the host. The named variants > > > are likely the ones specialized for various accelerators. > > > > > > Regardless, this is exactly why we should do this in Sema. We can > > > represent links to both Decls in the AST (as I indicated in an earlier > > > comment), and then it will be *possible* for an OpenMP-aware tool to > > > decide on which it wants. Right now, it's not easily possible to write a > > > tool that can use an appropriate set of contexts to examine the AST where > > > the actual callees are represented. > > > > > > No need to worry for the right decl. See D7097 > > <https://reviews.llvm.org/D7097>. If you see a refernce for function, > > before doing something with it, just call member function > > `getOpenMPDeclareVariantFunction()` and you get the function that must be > > actually called here. The tool must do the same. That's hiw the tools work. > > They must be aware of special costructs/attributes. > > > But then we'd to add code to do that in Clang's static analyzer and all other > code trying to match caller/callee relationships. The function provided in > the AST being not the function that will actually be called. Instead, we > should make these tools correct by default and make tools wanting to to > OpenMP-specific things be the tools that need to call the OpenMP-specific > functions. In general, yes, but not necessarily. We can teach existing functions like getBody(), isDefined() etc. about this feature and thus tools will get the correct function automatically. But I suggest to discuss this with Richard Smith. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71241/new/ https://reviews.llvm.org/D71241 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
