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

jdoerfert abandoned this revision. jdoerfert added a comment. D75779 is the proper implementation of the OpenMP standard. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71241/new/ https://reviews.llvm.org/D71241

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

ABataev added a comment. In D71241#1870218 , @rsmith wrote: > In D71241#1788003 , @jdoerfert wrote: > > > In D71241#1787652 , @hfinkel wrote: > > > > > In D71241#1787571

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

rsmith added a comment. In D71241#1788003 , @jdoerfert wrote: > In D71241#1787652 , @hfinkel wrote: > > > In D71241#1787571 , @ABataev wrote: > > > > > In D71241#1787265

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

ABataev added a comment. In D71241#1788003 , @jdoerfert wrote: > In D71241#1787652 , @hfinkel wrote: > > > In D71241#1787571 , @ABataev wrote: > > > > > In D71241#1787265

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

jdoerfert added a comment. In D71241#1787652 , @hfinkel wrote: > In D71241#1787571 , @ABataev wrote: > > > In D71241#1787265 , @hfinkel wrote: > > > > > In D71241#1786959

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

ABataev added a comment. In D71241#1787998 , @jdoerfert wrote: > In D71241#1787888 , @ABataev wrote: > > > Hal, are we going to support something like this? > > > I'm not Hal but I will answer anyway. > > > void

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

jdoerfert added a comment. In D71241#1787888 , @ABataev wrote: > Hal, are we going to support something like this? I'm not Hal but I will answer anyway. > void cpu() { asm("nop"); } > > #pragma omp declare variant(cpu) match(device =

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

ABataev added a comment. Hal, are we going to support something like this? void cpu() { asm("nop"); } #pragma omp declare variant(cpu) match(device = {kind(cpu)}) void wrong_asm() { asm ("xxx"); } Currently, there is an error when we try to emit the assembler output.

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

hfinkel added a comment. In D71241#1787571 , @ABataev wrote: > In D71241#1787265 , @hfinkel wrote: > > > In D71241#1786959 , @jdoerfert > > wrote: > > > > > In

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

ABataev added a comment. In D71241#1787265 , @hfinkel wrote: > In D71241#1786959 , @jdoerfert wrote: > > > In D71241#1786530 , @ABataev wrote: > > > > > Most probably, we

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

jdoerfert added a comment. In D71241#1787265 , @hfinkel wrote: > In D71241#1786959 , @jdoerfert wrote: > > > In D71241#1786530 , @ABataev wrote: > > > > > Most probably, we

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

hfinkel added a comment. In D71241#1786959 , @jdoerfert wrote: > In D71241#1786530 , @ABataev wrote: > > > Most probably, we can use this solution without adding a new expression. > > `DeclRefExpr` class can

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

jdoerfert added a comment. In D71241#1786530 , @ABataev wrote: > Most probably, we can use this solution without adding a new expression. > `DeclRefExpr` class can contain 2 decls: FoundDecl and the Decl being used. > You can use FoundDecl to point to

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

ABataev added a comment. Most probably, we can use this solution without adding a new expression. `DeclRefExpr` class can contain 2 decls: FoundDecl and the Decl being used. You can use FoundDecl to point to the original function and used decl to point to the function being called in this

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

jdoerfert added a comment. In D71241#1783362 , @ABataev wrote: > >>> - Take https://godbolt.org/z/2evvtN which shows that the alias solution > >>> is incompatible with linking. > >> > >> Undefined behavior according to the standard. > > > > I don't

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

JonChesterfield added a comment. Has anyone actually asked Richard to look at this? He isn't subscribed to the diff and may not be watching openmp-dev. I don't think it's reasonable to stall progress on optimising openmp indefinitely. Richard may find it difficult to find time to resolve this.

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

ABataev added a comment. In D71241#1783499 , @hfinkel wrote: > In D71241#1783444 , @ABataev wrote: > > > In D71241#1782586 , @hfinkel wrote: > > > > > In D71241#1782460

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

hfinkel added a comment. In D71241#1783444 , @ABataev wrote: > In D71241#1782586 , @hfinkel wrote: > > > In D71241#1782460 , > > @JonChesterfield wrote: > > > > > >

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

ABataev added a comment. In D71241#1782586 , @hfinkel wrote: > In D71241#1782460 , @JonChesterfield > wrote: > > > > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > > > > > Faithfulness¶ >

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

ABataev added a comment. In D71241#1782963 , @jdoerfert wrote: > In D71241#1782668 , @ABataev wrote: > > > In D71241#1782650 , @jdoerfert > > wrote: > > > > > While we talk

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

jdoerfert added a comment. In D71241#1782917 , @ABataev wrote: > In D71241#1782898 , @JonChesterfield > wrote: > > > In D71241#1782846 , @ABataev wrote: > > > > > But I

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

jdoerfert added a comment. In D71241#1782668 , @ABataev wrote: > In D71241#1782650 , @jdoerfert wrote: > > > While we talk a lot about what you think is bad about this solution it > > seems we ignore the problems

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

ABataev added a comment. In D71241#1782898 , @JonChesterfield wrote: > In D71241#1782846 , @ABataev wrote: > > > But I suggest to discuss this with Richard Smith. > > > Is the appeal to authority necessary to

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

JonChesterfield added a comment. In D71241#1782846 , @ABataev wrote: > But I suggest to discuss this with Richard Smith. Is the appeal to authority necessary to resolve this? The last few posts by Hal look clear to me. Especially convincing is: >

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

ABataev added a comment. In D71241#1782812 , @hfinkel wrote: > In D71241#1782779 , @ABataev wrote: > > > In D71241#1782742 , @hfinkel wrote: > > > > > In D71241#1782703

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

hfinkel added a comment. In D71241#1782779 , @ABataev wrote: > In D71241#1782742 , @hfinkel wrote: > > > In D71241#1782703 , @ABataev wrote: > > > > > > > > > > > ... > > >

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

hfinkel added a comment. In D71241#1782723 , @ABataev wrote: > I don't insist on function redefinition solution. You want to replace > functions - fine, but do this at the codegen, not in AST. Again, no one is replacing anything, and we're not mutating

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

ABataev added a comment. In D71241#1782742 , @hfinkel wrote: > In D71241#1782703 , @ABataev wrote: > > > > > > ... > > >> > >> > >>> Given we have two implementations, each at different points in the

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

hfinkel added a comment. In 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

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

ABataev added a comment. I don't insist on function redefinition solution. You want to replace functions - fine, but do this at the codegen, not in AST. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71241/new/ https://reviews.llvm.org/D71241

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

ABataev added a comment. In D71241#1782670 , @hfinkel wrote: > In D71241#1782652 , @ABataev wrote: > > > In D71241#1782648 , @hfinkel wrote: > > > > > In D71241#1782614

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

ABataev added a comment. In D71241#1782700 , @hfinkel wrote: > In D71241#1782668 , @ABataev wrote: > > > > > > ... > > >> While we talk a lot about what you think is bad about this solution it > >> seems we ignore

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

hfinkel added a comment. In D71241#1782668 , @ABataev wrote: > ... >> While we talk a lot about what you think is bad about this solution it seems >> we ignore the problems in the current one. Let me summarize a few: >> >> - Take

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

hfinkel added a comment. In D71241#1782652 , @ABataev wrote: > In D71241#1782648 , @hfinkel wrote: > > > In D71241#1782614 , @ABataev wrote: > > > > > In D71241#1782551

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

ABataev added a comment. In D71241#1782650 , @jdoerfert wrote: > In D71241#1782614 , @ABataev wrote: > > > Actually, early resolution will break tbe tools, not help them. It will > > definitely break clangd, for

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

hfinkel added a comment. In D71241#1782430 , @JonChesterfield wrote: > In D71241#1782427 , @ABataev wrote: > > > In D71241#1782425 , > > @JonChesterfield wrote: > > > > >

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

ABataev added a comment. In D71241#1782648 , @hfinkel wrote: > In D71241#1782614 , @ABataev wrote: > > > In D71241#1782551 , @hfinkel wrote: > > > > > In D71241#1779168

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

jdoerfert added a comment. In D71241#1782614 , @ABataev wrote: > 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

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

hfinkel added a comment. In D71241#1778671 , @ABataev wrote: > There can be another one issue with this solution with inline assembly. I’m > not completely sure about it, will try to investigate it tomorrow. I suggest > to discuss this solution with

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

hfinkel added a comment. In D71241#1782614 , @ABataev wrote: > In D71241#1782551 , @hfinkel wrote: > > > In D71241#1779168 , > > @JonChesterfield wrote: > > > > > Lowering

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

ABataev added a comment. In D71241#1782551 , @hfinkel wrote: > In D71241#1779168 , @JonChesterfield > wrote: > > > Lowering in sema or in codegen seems a standard phase ordering choice. > > There will be pros and

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

ABataev added a comment. In D71241#1782504 , @jdoerfert wrote: > In D71241#1782460 , @JonChesterfield > wrote: > > > > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > > > > > Faithfulness¶

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

hfinkel added a comment. In D71241#1779779 , @ABataev wrote: > Here is the example that does not work with the proposed solution but works > with the existing one: > > static void cpu() { asm("nop"); } > > #pragma omp declare variant(cpu)

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

hfinkel added a comment. In D71241#1782460 , @JonChesterfield wrote: > > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > > > Faithfulness¶ > > The AST intends to provide a representation of the program that is > > faithful to

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

hfinkel added a comment. In D71241#1779168 , @JonChesterfield wrote: > Lowering in sema or in codegen seems a standard phase ordering choice. There > will be pros and cons to both. > > I think prior art leans towards sema. Variants are loosely

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

jdoerfert added a comment. In D71241#1782460 , @JonChesterfield wrote: > > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > > > Faithfulness¶ > > The AST intends to provide a representation of the program that is > > faithful to

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

JonChesterfield added a comment. > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > Faithfulness¶ > The AST intends to provide a representation of the program that is faithful > to the original source. That's pretty convincing. Repository: rG LLVM Github Monorepo

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

ABataev added a comment. In D71241#1782430 , @JonChesterfield wrote: > In D71241#1782427 , @ABataev wrote: > > > In D71241#1782425 , > > @JonChesterfield wrote: > > > > >

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

JonChesterfield added a comment. In D71241#1782427 , @ABataev wrote: > In D71241#1782425 , @JonChesterfield > wrote: > > > > Explain that you're replacing the function written by the user on the fly > > > by

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

ABataev added a comment. In D71241#1782425 , @JonChesterfield wrote: > > Explain that you're replacing the function written by the user on the fly > > by another one. If they accept it, go ahead. > > That's the observational effect of variants.

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

JonChesterfield added a comment. > Explain that you're replacing the function written by the user on the fly by > another one. If they accept it, go ahead. That's the observational effect of variants. Replacing is very similar to calling + inlining. Repository: rG LLVM Github Monorepo

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

ABataev added a comment. In D71241#1782317 , @jdoerfert wrote: > In D71241#1782173 , @ABataev wrote: > > > In D71241#1782157 , @jdoerfert > > wrote: > > > > > In

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

jdoerfert added a comment. >> In fact, the current solution will disregard the `used` attribute here and >> not emit the function, which is bad. The Sema solution will dispatch the >> right calls and honor the `used` attribute properly. > > Nope, the function is emitted but as an alias to the

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

ABataev added a comment. In D71241#1782317 , @jdoerfert wrote: > In D71241#1782173 , @ABataev wrote: > > > In D71241#1782157 , @jdoerfert > > wrote: > > > > > In

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

jdoerfert added a comment. In D71241#1782173 , @ABataev wrote: > In D71241#1782157 , @jdoerfert wrote: > > > In D71241#1781955 , @ABataev wrote: > > > > > In D71241#1780715

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

ABataev added a comment. In D71241#1782157 , @jdoerfert wrote: > In D71241#1781955 , @ABataev wrote: > > > In D71241#1780715 , @jdoerfert > > wrote: > > > > > In

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

jdoerfert added a comment. In D71241#1781955 , @ABataev wrote: > In D71241#1780715 , @jdoerfert wrote: > > > In D71241#1779097 , @ABataev wrote: > > > > > In D71241#1778963

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

ABataev added a comment. In D71241#1780715 , @jdoerfert wrote: > In D71241#1779097 , @ABataev wrote: > > > In D71241#1778963 , @jdoerfert > > wrote: > > > > > In

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

jdoerfert added a comment. In D71241#1779097 , @ABataev wrote: > In D71241#1778963 , @jdoerfert wrote: > > > In D71241#1778736 , @ABataev wrote: > > > > > In D71241#1778717

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

ABataev added a comment. Here is the example that does not work with the proposed solution but works with the existing one: static void cpu() { asm("nop"); } #pragma omp declare variant(cpu) match(device = {kind(cpu)}) static __attribute__((used)) void wrong_asm() { asm ("xxx");

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

ABataev added a comment. In D71241#1779168 , @JonChesterfield wrote: > Lowering in sema or in codegen seems a standard phase ordering choice. There > will be pros and cons to both. > > I think prior art leans towards sema. Variants are loosely

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

JonChesterfield added a comment. Lowering in sema or in codegen seems a standard phase ordering choice. There will be pros and cons to both. I think prior art leans towards sema. Variants are loosely equivalent to tag dispatching or constexpr if, both handled before lowering the AST to IR.

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

ABataev added a comment. In D71241#1778963 , @jdoerfert wrote: > In D71241#1778736 , @ABataev wrote: > > > In D71241#1778717 , @jdoerfert > > wrote: > > > > > >> There is

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

jdoerfert added a comment. In D71241#1778736 , @ABataev wrote: > In D71241#1778717 , @jdoerfert wrote: > > > >> There is no evidence that this is more complicated. By all measures, > > >> this is less complicated

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

ABataev added a comment. In D71241#1778717 , @jdoerfert wrote: > >> There is no evidence that this is more complicated. By all measures, this > >> is less complicated (see also below). It is also actually doing the right > >> thing when it comes to code

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

jdoerfert added a comment. >> There is no evidence that this is more complicated. By all measures, this is >> less complicated (see also below). It is also actually doing the right thing >> when it comes to code emission. Take https://godbolt.org/z/sJiP3B for >> example. The calls are wrong

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

ABataev added a comment. There can be another one issue with this solution with inline assembly. I’m not completely sure about it, will try to investigate it tomorrow. I suggest to discuss this solution with Richard Smith (or John McCall). If he/they are ok with this transformation of the AST,

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

ABataev added a comment. In D71241#1778564 , @hfinkel wrote: > In D71241#1778134 , @ABataev wrote: > > > > > > ... > > >> > >> > >>> Also, check how -ast-print works with your solution. It returns different >

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

ABataev added a comment. In D71241#1778564 , @hfinkel wrote: > In D71241#1778134 , @ABataev wrote: > > > > > > ... > > >> > >> > >>> Also, check how -ast-print works with your solution. It returns different >

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

hfinkel added a comment. In 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

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

ABataev added a comment. In D71241#1777972 , @jdoerfert wrote: > In D71241#109 , @ABataev wrote: > > > In D71241#1777661 , @jdoerfert > > wrote: > > > > > In

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

jdoerfert marked an inline comment as done. jdoerfert added a comment. In D71241#109 , @ABataev wrote: > In D71241#1777661 , @jdoerfert wrote: > > > In D71241#1776798 ,

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

ABataev added a comment. In D71241#1777661 , @jdoerfert wrote: > In D71241#1776798 , @ABataev wrote: > > > You're merging different functions as multiversiin variants. I don't think > > this right to

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

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D71241#1776798 , @ABataev wrote: > You're merging different functions as multiversiin variants. I don't think > this right to overcomplicate the semantics of multiversion

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

ABataev added a comment. You're merging different functions as multiversiin variants. I don't think this right to overcomplicate the semantics of multiversion functions just because you want to do it. Comment at: clang/lib/Sema/SemaOverload.cpp:9725 + + // TODO: Handle

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

merge_guards_bot added a comment. Build result: fail - 60666 tests passed, 1 failed and 726 were skipped. failed: Clang.OpenMP/declare_variant_ast_print.cpp Log files: console-log.txt , CMakeCache.txt

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

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: s.egerton, guansong, bollu, simoncook, fedor.sergeev, aheejin, rampitec, jholewinski. Herald added