[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
hfinkel added inline comments. Comment at: clang/lib/AST/DeclarationName.cpp:147 +Name = Name.split(getOpenMPVariantManglingSeparatorStr()).first; + OS << Name; +} Would it make sense to print " (omp variant)" after the name to disambiguate? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
jdoerfert added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396 + IdentifierInfo &VariangtII = Context.Idents.get( + (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str()); + D.SetIdentifier(&VariangtII, D.getBeginLoc()); hfinkel wrote: > jdoerfert wrote: > > hfinkel wrote: > > > Is there any way in which this name might become visible to users (e.g., > > > in error messages)? > > Yes, I think so. One way to trigger it would be to define the same function > > in the same `omp begin declare variant scope` (or two with the same > > context). I haven't verified this though. > > TBH, I'm unsure how bad this actually is in the short term. The original > > name is still a at the beginning. We should obviously make sure the error > > message is appropriate eventually, e.g., it de-mangles the name. > I think that we have to consider diagnostic quality as a first-class citizen. > There are a few different ways that we might approach this. A minimal diff > from what you have might be: > > 1. Replace the "." above with ".ompvariant." > 2. Add logic to FunctionDecl::getNameForDiagnostic (or > NamedDecl::getNameForDiagnostic) that recognizes that magic string in the > name and alters the printing to do something intelligible for the user with > the name. > > (I think that (1) is a good idea anyway). > > Another way would be to add some first-class property to the function and > then leave the mangling to CodeGen. Are we mangling these in Sema in other > places too? > > > > > Also, these can have external linkage, right? I added a test for this in `begin_declare_variant_messages.c` and confirmed my earlier statement. Then I implemented what you describe above (in spirit). > Another way would be to add some first-class property to the function and > then leave the mangling to CodeGen. Are we mangling these in Sema in other > places too? That would potentially work but require us to touch a lot of places that deal with redefinitions and overload resolution. Getting the right clashes between specialized versions with the same name and openmp context but not getting the clashes for the same name but different openmp context will be non-trivial. Similarly, we need to ignore specialized versions during overload resolution, etc. etc. FWIW, this is design 8 or 9, or even more. I tried to keep the names till codegen in the beginning, using an approach similar to what you described. I tried to use namespaces (even in C) to get the right kind of name clashes and interactions, I tried ... This is the only thing that (so far) worked reliably and is (IMHO) very non-intrusive. > Also, these can have external linkage, right? Yes. My name clash test has 3 different linkages, see `begin_declare_variant_messages.c`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
hfinkel added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396 + IdentifierInfo &VariangtII = Context.Idents.get( + (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str()); + D.SetIdentifier(&VariangtII, D.getBeginLoc()); jdoerfert wrote: > hfinkel wrote: > > Is there any way in which this name might become visible to users (e.g., in > > error messages)? > Yes, I think so. One way to trigger it would be to define the same function > in the same `omp begin declare variant scope` (or two with the same context). > I haven't verified this though. > TBH, I'm unsure how bad this actually is in the short term. The original name > is still a at the beginning. We should obviously make sure the error message > is appropriate eventually, e.g., it de-mangles the name. I think that we have to consider diagnostic quality as a first-class citizen. There are a few different ways that we might approach this. A minimal diff from what you have might be: 1. Replace the "." above with ".ompvariant." 2. Add logic to FunctionDecl::getNameForDiagnostic (or NamedDecl::getNameForDiagnostic) that recognizes that magic string in the name and alters the printing to do something intelligible for the user with the name. (I think that (1) is a good idea anyway). Another way would be to add some first-class property to the function and then leave the mangling to CodeGen. Are we mangling these in Sema in other places too? Also, these can have external linkage, right? Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487 + +VMIs.erase(VMIs.begin() + BestIdx); +Exprs.erase(Exprs.begin() + BestIdx); jdoerfert wrote: > hfinkel wrote: > > Why is the ordering here useful? Don't you collect all of the variant > > clauses from all of the declarations? Can't there be duplicates? (and does > > the relative order need always be the same?) Are we effectively supporting > > here, as an extension, cases where not all of the declarations have the > > same set of variants declared (it loos like we are because there's no break > > in the `while (CalleeFnDecl)` loop, but this makes me wonder if that would > > still have an odd behavior. > > Why is the ordering here useful? > > I don't think it is ordered. We have a conceptual set and BestIdx determines > the which of the set is the best. All others are equal. > > > Don't you collect all of the variant clauses from all of the declarations? > > Yes. > > > Can't there be duplicates? (and does the relative order need always be the > > same?) > > Yes (and no). `getBestVariantMatchForContext` should determine the best > regardless of duplicates, we might just try it multiple times if we didn't > manage to create a call. > > > Are we effectively supporting here, as an extension, cases where not all of > > the declarations have the same set of variants declared (it loos like we > > are because there's no break in the while (CalleeFnDecl) loop, but this > > makes me wonder if that would still have an odd behavior. > > We are supporting that. All declarations are scanned and all variants are > collected. What odd behavior do you refer to? > Makes sense, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
jdoerfert added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396 + IdentifierInfo &VariangtII = Context.Idents.get( + (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str()); + D.SetIdentifier(&VariangtII, D.getBeginLoc()); hfinkel wrote: > Is there any way in which this name might become visible to users (e.g., in > error messages)? Yes, I think so. One way to trigger it would be to define the same function in the same `omp begin declare variant scope` (or two with the same context). I haven't verified this though. TBH, I'm unsure how bad this actually is in the short term. The original name is still a at the beginning. We should obviously make sure the error message is appropriate eventually, e.g., it de-mangles the name. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5471 + // variant expression. We allow this to fail in which case we continue with + // the next best variant expression. + Sema::TentativeAnalysisScope Trap(*this); hfinkel wrote: > Please provide an example or otherwise explain why this failure behavior is > correct. Tried to improve the comment. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487 + +VMIs.erase(VMIs.begin() + BestIdx); +Exprs.erase(Exprs.begin() + BestIdx); hfinkel wrote: > Why is the ordering here useful? Don't you collect all of the variant clauses > from all of the declarations? Can't there be duplicates? (and does the > relative order need always be the same?) Are we effectively supporting here, > as an extension, cases where not all of the declarations have the same set of > variants declared (it loos like we are because there's no break in the `while > (CalleeFnDecl)` loop, but this makes me wonder if that would still have an > odd behavior. > Why is the ordering here useful? I don't think it is ordered. We have a conceptual set and BestIdx determines the which of the set is the best. All others are equal. > Don't you collect all of the variant clauses from all of the declarations? Yes. > Can't there be duplicates? (and does the relative order need always be the > same?) Yes (and no). `getBestVariantMatchForContext` should determine the best regardless of duplicates, we might just try it multiple times if we didn't manage to create a call. > Are we effectively supporting here, as an extension, cases where not all of > the declarations have the same set of variants declared (it loos like we are > because there's no break in the while (CalleeFnDecl) loop, but this makes me > wonder if that would still have an odd behavior. We are supporting that. All declarations are scanned and all variants are collected. What odd behavior do you refer to? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")
hfinkel added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1253 +: Error<"expected '#pragma omp begin declare variant' to match this stray " +"`#pragma omp end delcare variant`">; def err_omp_declare_target_unexpected_clause: Error< We're not extremely consistent about how we phrase these kinds of errors, but I grepped for stray and we have only one message about stray tokens, but grepped for matching and we have a bunch more. Phrasing this as: '#pragma omp end declare variant' with no matching ''#pragma omp begin declare variant' might be better. Comment at: clang/include/clang/Sema/Sema.h:9727 + + /// The declarator \p D defines a function in the socpe \p S which is nested + /// in an `omp begin/end declare variant` scope. In this method we create a scope Comment at: clang/lib/Sema/SemaDecl.cpp:13552 + if (LangOpts.OpenMP && isInOpenMPDeclareVariantScope() && + TemplateParameterLists.empty()) +BaseFD = ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope( Please add a comment here explaining why the `TemplateParameterLists.empty()` check is here. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396 + IdentifierInfo &VariangtII = Context.Idents.get( + (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str()); + D.SetIdentifier(&VariangtII, D.getBeginLoc()); Is there any way in which this name might become visible to users (e.g., in error messages)? Comment at: clang/lib/Sema/SemaOpenMP.cpp:5471 + // variant expression. We allow this to fail in which case we continue with + // the next best variant expression. + Sema::TentativeAnalysisScope Trap(*this); Please provide an example or otherwise explain why this failure behavior is correct. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487 + +VMIs.erase(VMIs.begin() + BestIdx); +Exprs.erase(Exprs.begin() + BestIdx); Why is the ordering here useful? Don't you collect all of the variant clauses from all of the declarations? Can't there be duplicates? (and does the relative order need always be the same?) Are we effectively supporting here, as an extension, cases where not all of the declarations have the same set of variants declared (it loos like we are because there's no break in the `while (CalleeFnDecl)` loop, but this makes me wonder if that would still have an odd behavior. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75779/new/ https://reviews.llvm.org/D75779 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
