Author: pgerell Date: 2026-10-02T06:36:10-04:00 New Revision: f718ebe873411a562ef6e5c27da36d75b7aaec98
URL: https://github.com/llvm/llvm-project/commit/f718ebe873411a562ef6e5c27da36d75b7aaec98 DIFF: https://github.com/llvm/llvm-project/commit/f718ebe873411a562ef6e5c27da36d75b7aaec98.diff LOG: [clang][OpenMP] Keep 'requires' directives read from an AST file (#220058) An OpenMP `requires` directive is recorded in Sema when the directive is parsed (`SemaOpenMP::ActOnOpenMPRequiresDirective`). Nothing repopulated that list from an AST file. A translation unit that gets its `requires` directive from a PCH or a module therefore behaves as if the directive were absent, and clang rejects valid code. `OMPRequiresDecl` is already serialized and eagerly deserialized, so the declaration is present in the AST. Only Sema's view of it was missing. ## Reproducer ```c++ // rev.h #pragma omp requires reverse_offload void foo(); ``` ```c++ // use.cpp void bar(int argc) { #pragma omp target device(ancestor : argc) foo(); } ``` ``` $ clang -cc1 -fopenmp -fopenmp-version=51 -fopenmp-targets=x86_64 -triple x86_64 \ -x c++ -emit-pch -o r.pch rev.h $ clang -cc1 -fopenmp -fopenmp-version=51 -fopenmp-targets=x86_64 -triple x86_64 \ -include-pch r.pch -fsyntax-only use.cpp use.cpp:2:20: error: device clause with ancestor device-modifier used without specifying 'requires reverse_offload' ``` The requirement is specified, so the diagnostic is a false positive. The same code in a single file compiles cleanly, because the directive is then parsed in this TU. `-fopenmp-targets=` matters. Without an offload target clang defers this diagnostic and then drops it, because a host compilation never flushes its deferred diagnostics. That is a separate bug, with a fix to follow, and it makes this false positive reachable in the default configuration too. `reverse_offload` is the case with a directly observable diagnostic. The same state feeds every `hasRequiresDeclWithClause` query, so `dynamic_allocators` (on a device compilation) and `unified_shared_memory` (which changes implicit data-sharing instead of emitting a diagnostic) read it as well. ## Fix Write the `requires` directives into the AST file and register them with Sema, following the existing `DeclsWithEffectsToVerify` pattern. This happens in `ASTReader::UpdateSema` rather than `ASTReader::InitializeSema` because `UpdateSema` runs after every AST file is loaded. A directive can come from a module loaded during the compilation rather than up front. Tests cover both the PCH and the module path. The contents of this PR is created with help from Claude Code and ChatGPT. --------- Co-authored-by: Alexey Bataev <[email protected]> Added: clang/test/OpenMP/requires_module.cpp clang/test/OpenMP/requires_pch.cpp Modified: clang/docs/ReleaseNotes.md clang/include/clang/Sema/SemaOpenMP.h clang/include/clang/Serialization/ASTBitCodes.h clang/include/clang/Serialization/ASTReader.h clang/include/clang/Serialization/ASTWriter.h clang/lib/Sema/SemaOpenMP.cpp clang/lib/Serialization/ASTReader.cpp clang/lib/Serialization/ASTWriter.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index c5eed2b035e66..acdf563f7d0bf 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -1043,6 +1043,8 @@ The `alpha.cplusplus.UseAfterLifetimeEnd` checker was renamed to `alpha.core.Use ### OpenMP Support +- Fixed an OpenMP `requires` directive read from a PCH or module losing its effect on + semantic checks, which caused spurious `reverse_offload` errors. - Added the OpenMP 6.1 `#pragma omp flatten` loop transformation and the `depth` clause. Flatten combines perfectly nested canonical loops into one loop. `depth(k)` selects how many outermost loops to combine (default 2). diff --git a/clang/include/clang/Sema/SemaOpenMP.h b/clang/include/clang/Sema/SemaOpenMP.h index b8cea41bf8b9e..36c43cd2be0a2 100644 --- a/clang/include/clang/Sema/SemaOpenMP.h +++ b/clang/include/clang/Sema/SemaOpenMP.h @@ -259,6 +259,12 @@ class SemaOpenMP : public SemaBase { /// Called on well-formed '#pragma omp requires'. DeclGroupPtrTy ActOnOpenMPRequiresDirective(SourceLocation Loc, ArrayRef<OMPClause *> ClauseList); + + /// Registers a 'requires' directive deserialized from an AST file. + void addRequiresDecl(OMPRequiresDecl *D); + + /// The 'requires' directives seen so far in this translation unit. + ArrayRef<const OMPRequiresDecl *> getRequiresDecls() const; /// Check restrictions on Requires directive OMPRequiresDecl *CheckOMPRequiresDecl(SourceLocation Loc, ArrayRef<OMPClause *> Clauses); diff --git a/clang/include/clang/Serialization/ASTBitCodes.h b/clang/include/clang/Serialization/ASTBitCodes.h index e253fc47cacdd..9c3c55084de5e 100644 --- a/clang/include/clang/Serialization/ASTBitCodes.h +++ b/clang/include/clang/Serialization/ASTBitCodes.h @@ -747,6 +747,9 @@ enum ASTRecordTypes { /// Record that encodes the number of submodules, their base ID in the AST /// file, and for each module the relative bit offset into the stream. SUBMODULE_METADATA = 80, + + /// Record code for the OpenMP 'requires' directives seen in the TU. + OMP_REQUIRES_DECLS = 81, }; /// Record types used within a source manager block. diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h index 0c8c92feee176..c06d70a340ad3 100644 --- a/clang/include/clang/Serialization/ASTReader.h +++ b/clang/include/clang/Serialization/ASTReader.h @@ -1052,6 +1052,9 @@ class ASTReader : public ExternalPreprocessorSource, /// The IDs of all decls with function effects to be checked. SmallVector<GlobalDeclID> DeclsWithEffectsToVerify; + /// OpenMP 'requires' directives read from the AST file. + SmallVector<GlobalDeclID> OpenMPRequiresDecls; + /// The RISC-V intrinsic pragma(including RVV, SiFive and Andes). SmallVector<bool, 3> RISCVVecIntrinsicPragma; diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 95ae8a6ba8c74..f69646d1ca0a9 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -648,6 +648,7 @@ class ASTWriter : public ASTDeserializationListener, void WritePackPragmaOptions(Sema &SemaRef); void WriteFloatControlPragmaOptions(Sema &SemaRef); void WriteDeclsWithEffectsToVerify(Sema &SemaRef); + void WriteOpenMPRequiresDecls(Sema &SemaRef); void WriteModuleFileExtension(Sema &SemaRef, ModuleFileExtensionWriter &Writer); void WriteRISCVIntrinsicPragmas(Sema &SemaRef); diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 479ba4c0e9a59..6ef86b8391f40 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -685,6 +685,10 @@ class DSAStackTy { /// Add requires decl to internal vector void addRequiresDecl(OMPRequiresDecl *RD) { RequiresDecls.push_back(RD); } + ArrayRef<const OMPRequiresDecl *> getRequiresDecls() const { + return RequiresDecls; + } + /// Checks if the defined 'requires' directive has specified type of clause. template <typename ClauseType> bool hasRequiresDeclWithClause() const { return llvm::any_of(RequiresDecls, [](const OMPRequiresDecl *D) { @@ -2080,6 +2084,14 @@ void SemaOpenMP::InitDataSharingAttributesStack() { #define DSAStack static_cast<DSAStackTy *>(VarDataSharingAttributesStack) +void SemaOpenMP::addRequiresDecl(OMPRequiresDecl *D) { + DSAStack->addRequiresDecl(D); +} + +ArrayRef<const OMPRequiresDecl *> SemaOpenMP::getRequiresDecls() const { + return DSAStack->getRequiresDecls(); +} + void SemaOpenMP::pushOpenMPFunctionRegion() { DSAStack->pushFunction(); } void SemaOpenMP::popOpenMPFunctionRegion(const FunctionScopeInfo *OldFSI) { diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index ca45d225fc4bd..27891d25ca666 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -26,6 +26,7 @@ #include "clang/AST/DeclFriend.h" #include "clang/AST/DeclGroup.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/DeclOpenMP.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/DeclarationName.h" #include "clang/AST/Expr.h" @@ -81,6 +82,7 @@ #include "clang/Sema/Sema.h" #include "clang/Sema/SemaCUDA.h" #include "clang/Sema/SemaObjC.h" +#include "clang/Sema/SemaOpenMP.h" #include "clang/Sema/SemaRISCV.h" #include "clang/Sema/Weak.h" #include "clang/Serialization/ASTBitCodes.h" @@ -4474,6 +4476,11 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F, DeclsWithEffectsToVerify.push_back(ReadDeclID(F, Record, I)); break; + case OMP_REQUIRES_DECLS: + for (unsigned I = 0, N = Record.size(); I != N; /*in loop*/) + OpenMPRequiresDecls.push_back(ReadDeclID(F, Record, I)); + break; + case OPENCL_EXTENSIONS: for (unsigned I = 0, E = Record.size(); I != E; ) { auto Name = ReadString(Record, I); @@ -9292,6 +9299,12 @@ void ASTReader::InitializeSema(Sema &S) { void ASTReader::UpdateSema() { assert(SemaObj && "no Sema to update"); + // UpdateSema() runs after each AST file is loaded, not only the first, so a + // 'requires' directive from a module is registered too. + for (GlobalDeclID ID : OpenMPRequiresDecls) + SemaObj->OpenMP().addRequiresDecl(cast<OMPRequiresDecl>(GetDecl(ID))); + OpenMPRequiresDecls.clear(); + // Load the offsets of the declarations that Sema references. // They will be lazily deserialized when needed. if (!SemaDeclRefs.empty()) { diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index c28acb398f66e..d76c3102b9792 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -24,6 +24,7 @@ #include "clang/AST/DeclContextInternals.h" #include "clang/AST/DeclFriend.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/DeclOpenMP.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/DeclarationName.h" #include "clang/AST/Expr.h" @@ -70,6 +71,7 @@ #include "clang/Sema/Sema.h" #include "clang/Sema/SemaCUDA.h" #include "clang/Sema/SemaObjC.h" +#include "clang/Sema/SemaOpenMP.h" #include "clang/Sema/SemaRISCV.h" #include "clang/Sema/Weak.h" #include "clang/Serialization/ASTBitCodes.h" @@ -5282,6 +5284,19 @@ void ASTWriter::WriteDeclsWithEffectsToVerify(Sema &SemaRef) { Stream.EmitRecord(DECLS_WITH_EFFECTS_TO_VERIFY, Record); } +/// Write the OpenMP 'requires' directives seen in this translation unit. +void ASTWriter::WriteOpenMPRequiresDecls(Sema &SemaRef) { + ArrayRef<const OMPRequiresDecl *> Decls = SemaRef.OpenMP().getRequiresDecls(); + if (Decls.empty()) + return; + RecordData Record; + for (const OMPRequiresDecl *D : Decls) + if (!D->isFromASTFile()) + AddDeclRef(D, Record); + if (!Record.empty()) + Stream.EmitRecord(OMP_REQUIRES_DECLS, Record); +} + void ASTWriter::WriteModuleFileExtension(Sema &SemaRef, ModuleFileExtensionWriter &Writer) { // Enter the extension block. @@ -6351,6 +6366,7 @@ ASTFileSignature ASTWriter::WriteASTCore(Sema *SemaPtr, StringRef isysroot, WritePackPragmaOptions(*SemaPtr); WriteFloatControlPragmaOptions(*SemaPtr); WriteDeclsWithEffectsToVerify(*SemaPtr); + WriteOpenMPRequiresDecls(*SemaPtr); } // Some simple statistics diff --git a/clang/test/OpenMP/requires_module.cpp b/clang/test/OpenMP/requires_module.cpp new file mode 100644 index 0000000000000..7e7034473b224 --- /dev/null +++ b/clang/test/OpenMP/requires_module.cpp @@ -0,0 +1,28 @@ +// RUN: rm -rf %t && split-file %s %t +// RUN: %clang_cc1 -fopenmp -fopenmp-version=51 -fmodules -fmodule-name=rev \ +// RUN: -x c++ -emit-module %t/module.modulemap -o %t/rev.pcm +// RUN: %clang_cc1 -fopenmp -fopenmp-version=51 -fmodules -fmodule-file=%t/rev.pcm \ +// RUN: -verify -fsyntax-only %t/use.cpp +// RUN: %clang_cc1 -fopenmp -fopenmp-version=51 -fopenmp-targets=x86_64 -triple x86_64 \ +// RUN: -fmodules -fmodule-name=rev -x c++ -emit-module %t/module.modulemap -o %t/rev2.pcm +// RUN: %clang_cc1 -fopenmp -fopenmp-version=51 -fopenmp-targets=x86_64 -triple x86_64 \ +// RUN: -fmodules -fmodule-file=%t/rev2.pcm -verify -fsyntax-only %t/use.cpp + +// A 'requires' directive read from a module must keep its effect on the +// translation unit importing it. + +//--- module.modulemap +module rev { header "rev.h" export * } + +//--- rev.h +#pragma omp requires reverse_offload +void foo(); + +//--- use.cpp +#include "rev.h" + +// expected-no-diagnostics +void bar(int argc) { +#pragma omp target device(ancestor : argc) + foo(); +} diff --git a/clang/test/OpenMP/requires_pch.cpp b/clang/test/OpenMP/requires_pch.cpp new file mode 100644 index 0000000000000..7f1e76252ac6c --- /dev/null +++ b/clang/test/OpenMP/requires_pch.cpp @@ -0,0 +1,24 @@ +// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=51 -x c++ -std=c++11 -emit-pch -o %t %s +// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=51 -std=c++11 -include-pch %t -fsyntax-only %s +// RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-version=51 -x c++ -std=c++11 -emit-pch -o %t %s +// RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-version=51 -std=c++11 -include-pch %t -fsyntax-only %s +// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=51 -fopenmp-targets=x86_64 \ +// RUN: -triple x86_64 -x c++ -std=c++11 -emit-pch -o %t %s +// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=51 -fopenmp-targets=x86_64 \ +// RUN: -triple x86_64 -std=c++11 -include-pch %t -fsyntax-only %s + +// expected-no-diagnostics + +// A 'requires' directive read from an AST file must keep its effect on the +// translation unit including it. + +#ifndef HEADER +#define HEADER +#pragma omp requires reverse_offload +void foo(); +#else +void bar(int argc) { +#pragma omp target device(ancestor : argc) + foo(); +} +#endif _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
