[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71241#1778564 <https://reviews.llvm.org/D71241#1778564>, @hfinkel wrote:

> In D71241#1778134 <https://reviews.llvm.org/D71241#1778134>, @ABataev wrote:
>
> >
>
>
> ...
>
> >> 
> >> 
> >>> Also, check how -ast-print works with your solution. It returns different result than expected because you're transform the code too early. It is incorrect behavior.
> >> 
> >> This is debatable. AST print does not print the input but the AST, thus what is correct wrt. OpenMP declare variant is nowhere defined but by us.
> >>  Arguably, having it print the actually called function and not the base function is preferable. Thus, the new way is actually more informative.
> > 
> > You're completely wrong here! We shall keep the original AST. This is used in several tools and you significantly modify the user code. I consider it a real issue here.
>
> Alexey, again, this kind of comment is not appropriate. We're all experienced developers here, and we all understand the importance of tooling support in Clang. We also serve developers who write tools using AST matchers and other Clang analysis facilities. Having the resolved callee represented in the AST for what looks like a static call from the base-language perspective makes a lot of sense from a tooling perspective. When performing static analysis on the code, forcing a tool to understand how OpenMP variant selectors work in order to perform inter-procedural static analysis is suboptimal in nearly all cases. It is also true that we might want the base callee represented in some way, but as that callee is never actually called, and one of the variants is always called at that call site, it is important that IPA propagate information into and out of the correct callee in order to produce the correct results. If we currently represent the base callee as the callee that will appear in the call graph, that's a bug: Clang's static analyzer will produce incorrect results.
>
> If you know of specific tools that indeed depend on the current behavior to produce correct results, please provide us with details on what they're doing so that we understand the use cases. Regardless, we should prioritize correct-by-default functioning of AST-based call graphs and their associated static analysis.




1. This is significant issue that you're modifiy the original user code.
2. It may have some troubles with serialization/deserialization probably. Plus, I just don't understand why it is good to rewrite the code for the user but to keep the original code is bad.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71241