Page MenuHomePhabricator

[OpenMP] Try to find an existing base for `omp begin/end declare variant`
ClosedPublic

Authored by jdoerfert on Apr 1 2020, 4:12 PM.

Details

Summary

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.

Diff Detail

Event Timeline

jdoerfert created this revision.Apr 1 2020, 4:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 4:12 PM
mikerice added inline comments.Apr 4 2020, 1:27 PM
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.

jdoerfert marked 2 inline comments as done.Apr 4 2020, 4:59 PM
jdoerfert added inline comments.
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:
CXX='clang++' CXXFLAGS='-fopenmp -fopenmp-targets=nvptx64-nvidia-cuda' ./omphval.sh run ./tests/math_cpp11
You'll see compile or link errors if it doesn't work for any reason. Doesn't mean we could not make it work by changing in the math wrappers but that depends on the errors (if any).

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.

[...]

 [...]
TypeSourceInfo *TInfo = GetTypeForDeclarator(D, S);
QualType FType = TInfo->getType();
[...]

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!

jdoerfert updated this revision to Diff 255124.Apr 5 2020, 12:10 AM

Simplify as suggeston by @mikerice, continue using the Declarator to create the implicit base if needed

mikerice accepted this revision.Apr 6 2020, 9:59 AM

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.

This revision is now accepted and ready to land.Apr 6 2020, 9:59 AM
This revision was automatically updated to reflect the committed changes.