If we have a function definition in omp begin/end declare variant it
is a specialization of a base function with the same name and
"compatible" type. Before, we just created a declaration for the base.
With this patch we try to find an existing declaration first and only
create a new one if we did not find any with a compatible type. This is
preferable as we can tolerate slight mismatches, especially if the
specialized version is "more constrained", e.g., constexpr.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5623–5624 | BaseFD is only implicit now if you created it, not when one was found in the lookup case right? | |
5629 | I think if you do the base lookup in ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope instead of here you might have better success using the declarator. You would need to get the type and constexpr/consteval info from the declarator and declspec but if the lookup fails you can just use ActOnDeclarator to get a base declaration like you were before. I tried this in my sandbox and it seems to work (AST tests fail I'm guessing that is just order change). If that works we could get rid of these hacky bits. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5623–5624 | Correct. Need to be changed. | |
5629 | Can you share how you do the lookup and the type comparison with the Declarator? I mean, name lookup is simple but determining the right overload wrt. type and the namespace was something I always had problems with. If you want to stress test your solution and don't mind, take the math patch and run the https://github.com/TApplencourt/OmpVal tests: |
I just moved your lookup code and tried to get the same info from the declarator. The function looks like this:
FunctionDecl * Sema::ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope(Scope *S, Declarator &D) { IdentifierInfo *BaseII = D.getIdentifier(); LookupResult Lookup(*this, DeclarationName(BaseII), D.getIdentifierLoc(), LookupOrdinaryName); LookupParsedName(Lookup, S, &D.getCXXScopeSpec()); TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S); QualType FType = TInfo->getType(); bool IsConstexpr = D.getDeclSpec().getConstexprSpecifier() == CSK_constexpr; bool IsConsteval = D.getDeclSpec().getConstexprSpecifier() == CSK_consteval; FunctionDecl *BaseFD = nullptr; for (auto *Candidate : Lookup) { auto *UDecl = dyn_cast<FunctionDecl>(Candidate->getUnderlyingDecl()); if (!UDecl) continue; // Don't specialize constexpr/consteval functions with // non-constexpr/consteval functions. if (UDecl->isConstexpr() && !IsConstexpr) continue; if (UDecl->isConsteval() && !IsConsteval) continue; QualType NewType = Context.mergeFunctionTypes( FType, UDecl->getType(), /* OfBlockPointer */ false, /* Unqualified */ false, /* AllowCXX */ true); if (NewType.isNull()) continue; // Found a base! BaseFD = UDecl; break; } if (!BaseFD) { BaseFD = cast<FunctionDecl>(ActOnDeclarator(S, D)); BaseFD->setImplicit(true); } OMPDeclareVariantScope &DVScope = OMPDeclareVariantScopes.back(); std::string MangledName; MangledName += D.getIdentifier()->getName(); MangledName += getOpenMPVariantManglingSeparatorStr(); MangledName += DVScope.NameSuffix; IdentifierInfo &VariantII = Context.Idents.get(MangledName); VariantII.setMangledOpenMPVariantName(true); D.SetIdentifier(&VariantII, D.getBeginLoc()); return BaseFD; }
I just tried a few tests so I didn't spend too much time verifying it. I'll see if I can get the test your suggest running.
I'm not sure I have any better ideas on how to correctly pick the base function. This seems like a reasonable first attempt.
I think this is a key part of what I was missing. I'll try out your patch with the all the tests and update the patch if it works (which I assume). Thanks!
Simplify as suggeston by @mikerice, continue using the Declarator to create the implicit base if needed
LGTM. I did run the math_cpp11 tests. I don't have a cuda environment so it was probably of limited value. I didn't see any failures that seemed related to this during the compiles.
BaseFD is only implicit now if you created it, not when one was found in the lookup case right?