Page MenuHomePhabricator

[OpenMP] Fix problems with the declare variant append_args clause
ClosedPublic

Authored by mikerice on Wed, Jan 12, 4:05 PM.

Details

Summary

Use ASTContext::getTypeDeclType() to get type of omp_interop_t since
TypeDecl::getTypeForDecl() may return null if TypeForDecl is not
setup yet.

Handle functions where the function type is under an AttributedType.

Diff Detail

Event Timeline

mikerice created this revision.Wed, Jan 12, 4:05 PM
mikerice requested review of this revision.Wed, Jan 12, 4:05 PM
ABataev added inline comments.Wed, Jan 12, 4:22 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

Do we always expect omp_interop_t to be a typedef?

mikerice added inline comments.Wed, Jan 12, 4:34 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

I guess I do. The spec says it must be an integral or pointer type. Could it be something other than a TypedefNameDecl?

ABataev added inline comments.Wed, Jan 12, 4:43 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

Can we check instead if it is compatible with integral or pointer type, of we drop all typedefs, using, etc?

mikerice added inline comments.Thu, Jan 13, 10:28 AM
clang/lib/Sema/SemaOpenMP.cpp
7068

Sorry I don't think I understand what you are requesting. TypedefNameDecl is the base class used for both typedefs and using decls so this would work for either. Unless there is some C++-only implementation the declaration in omp.h is likely to be a typedef though. I think we'd rather not drop typedefs here so diagnostics are clearer if this shows up in a diagnostic.

ABataev added inline comments.Thu, Jan 13, 11:21 AM
clang/lib/Sema/SemaOpenMP.cpp
7068

I suggest to desugar omp_interop_t type and use it’s main (integral or pointer) type to be fully compatible with any other runtime library inpelementation/OpenMP specification. And use this desugared type as a type of the extra parameter.

mikerice added inline comments.Thu, Jan 13, 2:22 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

I can do that but are you sure we should? What kind of sugar affects type compatibility? If we do this we get a diagnostic that no longer mentions omp_interop_t and the user will have a harder time understanding the situation.

Old diagnostic:
variant in '#pragma omp declare variant' with type 'void (A::*)(float *, float *, int *)' is incompatible with type 'void (A::*)(float *, float *, int *, omp_interop_t)' with appended arguments

New diagnostic:
variant in '#pragma omp declare variant' with type 'void (A::*)(float *, float *, int *)' is incompatible with type 'void (A::*)(float *, float *, int *, void *)' with appended arguments

ABataev added inline comments.Thu, Jan 13, 2:57 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

Ok, I see. And what's current behavior for C code?

mikerice added inline comments.Thu, Jan 13, 3:26 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

The problem that I am trying to fix is just that using TypeDecl::getTypeForDecl() sometimes returns null here crashing the compiler. It seems the ASTContext members should be used instead since they check and set TypeForDecl if needed.

This shows up in some C cases but not in a C++ compile of the same code. I guess there is some subtle difference in how typedefs are handled in C vs. C++.

ABataev added inline comments.Thu, Jan 13, 3:29 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

Could you check please, why does it return nullptr?

mikerice added inline comments.Thu, Jan 13, 4:41 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

There is a difference in how name lookup is working in C and C++.

In C it is simple: lookup finds a single Decl and returns it without setting TypeForDecl.

In C++ two types of lookups occur and the typedef is found in both. This leaves two Decls that need to be resolved. During that processing ASTContext::getTypeDeclType is called and sets TypeForDecl (around line 528 of SemaLookup.cpp).

ABataev added inline comments.Thu, Jan 13, 5:15 PM
clang/lib/Sema/SemaOpenMP.cpp
7068

Can we mimic same behavior for C only? I mean, keep existing code, but if it is C manually call ASTContext::getTypeDeclsForType?

mikerice added inline comments.Fri, Jan 14, 8:52 AM
clang/lib/Sema/SemaOpenMP.cpp
7068

I think the right thing to do is just replace the getTypeForDecl call with a getTypeDeclType call. This will do the right thing after a C lookup and doesn't change the behavior for C++ (internally it will just return TypeForDecl). I'll upload a new patch to look at.

mikerice updated this revision to Diff 400035.Fri, Jan 14, 8:54 AM
mikerice edited the summary of this revision. (Show Details)

Addressed review comments.

This revision is now accepted and ready to land.Fri, Jan 14, 9:08 AM
aaron.ballman accepted this revision.Fri, Jan 14, 9:11 AM

LGTM modulo a formatting nit with the test.

clang/lib/Sema/SemaOpenMP.cpp
7068

I think the right thing to do is just replace the getTypeForDecl call with a getTypeDeclType call.

I agree.

clang/test/OpenMP/declare_variant_clauses_ast_print.c
2

Can you add a whitespace after the comment marker (in the whole file)? Then my eyes won't bleed so badly.

mikerice updated this revision to Diff 400045.Fri, Jan 14, 9:21 AM

Added whitespace to the test.

This revision was landed with ongoing or failed builds.Fri, Jan 14, 11:15 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jan 14, 11:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript