This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][FIX] Allow declare variant to work with reference types
ClosedPublic

Authored by jdoerfert on Aug 26 2021, 11:19 AM.

Details

Summary

Reference types in the return or parameter position did cause the OpenMP
declare variant overload reasoning to give up. We should allow them as
we allow any other type.

This should fix the bug reported on the mailing list:
https://lists.llvm.org/pipermail/openmp-dev/2021-August/004094.html

Diff Detail

Event Timeline

jdoerfert created this revision.Aug 26 2021, 11:19 AM
jdoerfert requested review of this revision.Aug 26 2021, 11:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2021, 11:19 AM
Herald added a subscriber: sstefan1. · View Herald Transcript

Clang format patch

ABataev added inline comments.Aug 26 2021, 12:36 PM
clang/lib/AST/ASTContext.cpp
9715

Does it matter if it is an LValueReferenceType or RValueReferenceType? What if one type is & ref kind and another is &&?

That is a surprising root cause. It turns out std::cexp does in fact take the complex argument by const reference. The C versions take it by value. Shall we guard this with if (openmp) or similar to avoid it changing semantics for other languages?

jdoerfert updated this revision to Diff 368978.Aug 26 2021, 2:09 PM

Add OpenMP check, add L/RValue check, add tests for multiple combinations of & vs &&

I think I addressed your concerns with the 2 additional checks and more test coverage, as always, return 0 are the functions it should call.

This revision is now accepted and ready to land.Aug 26 2021, 2:24 PM
pdhaliwal accepted this revision.Aug 26 2021, 11:16 PM

Confirmed locally that this fixes the linking issue. It has fixed the linking issue on amdgcn as well. Thanks for working on this.