Skip to content

[clang][OpenMP] Add 'align' modifier for 'allocate' clause #121814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

ddpagan
Copy link
Contributor

@ddpagan ddpagan commented Jan 6, 2025

The 'align' modifier is now accepted in the 'allocate' clause. Added LIT tests covering codegen, PCH, template handling, and serialization for 'align' modifier.

Added support for align-modifier to release notes.

Testing

  • New allocate modifier LIT tests.
  • OpenMP LIT tests.
  • check-all

@ddpagan ddpagan requested a review from alexey-bataev January 6, 2025 18:41
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:openmp OpenMP related changes to Clang labels Jan 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 6, 2025

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: David Pagan (ddpagan)

Changes

The 'align' modifier is now accepted in the 'allocate' clause. Added LIT tests covering codegen, PCH, template handling, and serialization for 'align' modifier.

Added support for align-modifier to release notes.

Testing

  • New allocate modifier LIT tests.
  • OpenMP LIT tests.
  • check-all

Patch is 91.26 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/121814.diff

17 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/include/clang/AST/OpenMPClause.h (+81-11)
  • (modified) clang/include/clang/Basic/DiagnosticParseKinds.td (+2)
  • (modified) clang/include/clang/Basic/OpenMPKinds.def (+1)
  • (modified) clang/include/clang/Basic/OpenMPKinds.h (+4)
  • (modified) clang/include/clang/Sema/SemaOpenMP.h (+15-5)
  • (modified) clang/lib/AST/OpenMPClause.cpp (+42-16)
  • (modified) clang/lib/Parse/ParseOpenMP.cpp (+75-20)
  • (modified) clang/lib/Sema/SemaOpenMP.cpp (+67-35)
  • (modified) clang/lib/Sema/TreeTransform.h (+21-9)
  • (modified) clang/lib/Serialization/ASTReader.cpp (+5-1)
  • (modified) clang/lib/Serialization/ASTWriter.cpp (+3-1)
  • (removed) clang/test/OpenMP/allocate_allocator_modifier_codegen.cpp (-255)
  • (removed) clang/test/OpenMP/allocate_allocator_modifier_messages.cpp (-97)
  • (renamed) clang/test/OpenMP/allocate_modifiers_ast_print.cpp (+76-1)
  • (added) clang/test/OpenMP/allocate_modifiers_codegen.cpp (+409)
  • (added) clang/test/OpenMP/allocate_modifiers_messages.cpp (+159)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index acd9dd9298ce1e..25d390f69bcea3 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1284,6 +1284,7 @@ OpenMP Support
 - Changed the OpenMP DeviceRTL to use 'generic' IR. The
   ``LIBOMPTARGET_DEVICE_ARCHITECTURES`` CMake argument is now unused and will
   always build support for AMDGPU and NVPTX targets.
+- Added support for align-modifier in 'allocate' clause.
 
 Improvements
 ^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/OpenMPClause.h b/clang/include/clang/AST/OpenMPClause.h
index d2f5267e4da5ea..b9088eff3bb52e 100644
--- a/clang/include/clang/AST/OpenMPClause.h
+++ b/clang/include/clang/AST/OpenMPClause.h
@@ -498,6 +498,9 @@ class OMPAllocateClause final
   /// Allocator specified in the clause, or 'nullptr' if the default one is
   /// used.
   Expr *Allocator = nullptr;
+  /// Alignment specified in the clause, or 'nullptr' if the default one is
+  /// used.
+  Expr *Alignment = nullptr;
   /// Position of the ':' delimiter in the clause;
   SourceLocation ColonLoc;
   /// Modifier of 'allocate' clause.
@@ -505,6 +508,41 @@ class OMPAllocateClause final
   /// Location of allocator modifier if any.
   SourceLocation AllocatorModifierLoc;
 
+  // ----------------------------------------------------------------------------
+
+  /// Modifiers for 'allocate' clause.
+  enum { FIRST, SECOND, NUM_MODIFIERS };
+  OpenMPAllocateClauseModifier Modifiers[NUM_MODIFIERS];
+
+  /// Locations of modifiers.
+  SourceLocation ModifiersLoc[NUM_MODIFIERS];
+
+  /// Set the first allocate modifier.
+  ///
+  /// \param M Allocate modifier.
+  void setFirstAllocateModifier(OpenMPAllocateClauseModifier M) {
+    Modifiers[FIRST] = M;
+  }
+
+  /// Set the second allocate modifier.
+  ///
+  /// \param M Allocate modifier.
+  void setSecondAllocateModifier(OpenMPAllocateClauseModifier M) {
+    Modifiers[SECOND] = M;
+  }
+
+  /// Set location of the first allocate modifier.
+  void setFirstAllocateModifierLoc(SourceLocation Loc) {
+    ModifiersLoc[FIRST] = Loc;
+  }
+
+  /// Set location of the second allocate modifier.
+  void setSecondAllocateModifierLoc(SourceLocation Loc) {
+    ModifiersLoc[SECOND] = Loc;
+  }
+
+  // ----------------------------------------------------------------------------
+
   /// Build clause with number of variables \a N.
   ///
   /// \param StartLoc Starting location of the clause.
@@ -514,15 +552,20 @@ class OMPAllocateClause final
   /// \param EndLoc Ending location of the clause.
   /// \param N Number of the variables in the clause.
   OMPAllocateClause(SourceLocation StartLoc, SourceLocation LParenLoc,
-                    Expr *Allocator, SourceLocation ColonLoc,
-                    OpenMPAllocateClauseModifier AllocatorModifier,
-                    SourceLocation AllocatorModifierLoc, SourceLocation EndLoc,
+                    Expr *Allocator, Expr *Alignment, SourceLocation ColonLoc,
+                    OpenMPAllocateClauseModifier Modifier1,
+                    SourceLocation Modifier1Loc,
+                    OpenMPAllocateClauseModifier Modifier2,
+                    SourceLocation Modifier2Loc, SourceLocation EndLoc,
                     unsigned N)
       : OMPVarListClause<OMPAllocateClause>(llvm::omp::OMPC_allocate, StartLoc,
                                             LParenLoc, EndLoc, N),
-        Allocator(Allocator), ColonLoc(ColonLoc),
-        AllocatorModifier(AllocatorModifier),
-        AllocatorModifierLoc(AllocatorModifierLoc) {}
+        Allocator(Allocator), Alignment(Alignment), ColonLoc(ColonLoc) {
+    Modifiers[FIRST] = Modifier1;
+    Modifiers[SECOND] = Modifier2;
+    ModifiersLoc[FIRST] = Modifier1Loc;
+    ModifiersLoc[SECOND] = Modifier2Loc;
+  }
 
   /// Build an empty clause.
   ///
@@ -530,7 +573,10 @@ class OMPAllocateClause final
   explicit OMPAllocateClause(unsigned N)
       : OMPVarListClause<OMPAllocateClause>(llvm::omp::OMPC_allocate,
                                             SourceLocation(), SourceLocation(),
-                                            SourceLocation(), N) {}
+                                            SourceLocation(), N) {
+    Modifiers[FIRST] = OMPC_ALLOCATE_unknown;
+    Modifiers[SECOND] = OMPC_ALLOCATE_unknown;
+  }
 
   /// Sets location of ':' symbol in clause.
   void setColonLoc(SourceLocation CL) { ColonLoc = CL; }
@@ -539,6 +585,7 @@ class OMPAllocateClause final
   void setAllocatorModifier(OpenMPAllocateClauseModifier AM) {
     AllocatorModifier = AM;
   }
+  void setAlignment(Expr *A) { Alignment = A; }
 
 public:
   /// Creates clause with a list of variables \a VL.
@@ -554,19 +601,42 @@ class OMPAllocateClause final
   /// \param VL List of references to the variables.
   static OMPAllocateClause *
   Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation LParenLoc,
-         Expr *Allocator, SourceLocation ColonLoc,
-         OpenMPAllocateClauseModifier AllocatorModifier,
-         SourceLocation AllocatorModifierLoc, SourceLocation EndLoc,
-         ArrayRef<Expr *> VL);
+         Expr *Allocator, Expr *Alignment, SourceLocation ColonLoc,
+         OpenMPAllocateClauseModifier Modifier1, SourceLocation Modifier1Loc,
+         OpenMPAllocateClauseModifier Modifier2, SourceLocation Modifier2Loc,
+         SourceLocation EndLoc, ArrayRef<Expr *> VL);
 
   /// Returns the allocator expression or nullptr, if no allocator is specified.
   Expr *getAllocator() const { return Allocator; }
 
+  /// Returns the alignment expression or nullptr, if no alignment specified.
+  Expr *getAlignment() const { return Alignment; }
+
   /// Return 'allocate' modifier.
   OpenMPAllocateClauseModifier getAllocatorModifier() const {
     return AllocatorModifier;
   }
 
+  /// Get the first modifier of the clause.
+  OpenMPAllocateClauseModifier getFirstAllocateModifier() const {
+    return Modifiers[FIRST];
+  }
+
+  /// Get location of first modifier of the clause.
+  SourceLocation getFirstAllocateModifierLoc() const {
+    return ModifiersLoc[FIRST];
+  }
+
+  /// Get the second modifier of the clause.
+  OpenMPAllocateClauseModifier getSecondAllocateModifier() const {
+    return Modifiers[SECOND];
+  }
+
+  /// Get location of second modifier of the clause.
+  SourceLocation getSecondAllocateModifierLoc() const {
+    return ModifiersLoc[SECOND];
+  }
+
   /// Returns the location of the ':' delimiter.
   SourceLocation getColonLoc() const { return ColonLoc; }
   /// Return the location of the modifier.
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 86fcae209c40db..3309f59a981fc1 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1658,6 +1658,8 @@ def warn_omp_depend_in_ordered_deprecated : Warning<"'depend' clause for"
 def warn_omp_invalid_attribute_for_ompx_attributes : Warning<"'ompx_attribute' clause only allows "
   "'amdgpu_flat_work_group_size', 'amdgpu_waves_per_eu', and 'launch_bounds'; "
   "%0 is ignored">, InGroup<OpenMPExtensions>;
+def err_omp_duplicate_modifier : Error<"duplicate modifier '%0' in '%1' clause">;
+def err_omp_expected_modifier : Error<"expected modifier in '%0' clause">;
 
 // Pragma loop support.
 def err_pragma_loop_missing_argument : Error<
diff --git a/clang/include/clang/Basic/OpenMPKinds.def b/clang/include/clang/Basic/OpenMPKinds.def
index 3f25e7aafe23b6..76a861f416fd57 100644
--- a/clang/include/clang/Basic/OpenMPKinds.def
+++ b/clang/include/clang/Basic/OpenMPKinds.def
@@ -219,6 +219,7 @@ OPENMP_NUMTASKS_MODIFIER(strict)
 
 // Modifiers for 'allocate' clause.
 OPENMP_ALLOCATE_MODIFIER(allocator)
+OPENMP_ALLOCATE_MODIFIER(align)
 
 // Modifiers for the 'doacross' clause.
 OPENMP_DOACROSS_MODIFIER(source)
diff --git a/clang/include/clang/Basic/OpenMPKinds.h b/clang/include/clang/Basic/OpenMPKinds.h
index 900ad6ca6d66f6..3e5da2a6abc017 100644
--- a/clang/include/clang/Basic/OpenMPKinds.h
+++ b/clang/include/clang/Basic/OpenMPKinds.h
@@ -230,6 +230,10 @@ enum OpenMPAllocateClauseModifier {
   OMPC_ALLOCATE_unknown
 };
 
+/// Number of allowed allocate-modifiers.
+static constexpr unsigned NumberOfOMPAllocateClauseModifiers =
+    OMPC_ALLOCATE_unknown;
+
 /// Contains 'interop' data for 'append_args' and 'init' clauses.
 class Expr;
 struct OMPInteropInfo final {
diff --git a/clang/include/clang/Sema/SemaOpenMP.h b/clang/include/clang/Sema/SemaOpenMP.h
index 3d1cc4fab1c10f..a056a96f502333 100644
--- a/clang/include/clang/Sema/SemaOpenMP.h
+++ b/clang/include/clang/Sema/SemaOpenMP.h
@@ -1148,7 +1148,12 @@ class SemaOpenMP : public SemaBase {
     SourceLocation OmpAllMemoryLoc;
     SourceLocation
         StepModifierLoc; /// 'step' modifier location for linear clause
-    OpenMPAllocateClauseModifier AllocClauseModifier = OMPC_ALLOCATE_unknown;
+    SmallVector<OpenMPAllocateClauseModifier,
+                NumberOfOMPAllocateClauseModifiers>
+        AllocClauseModifiers;
+    SmallVector<SourceLocation, NumberOfOMPAllocateClauseModifiers>
+        AllocClauseModifiersLoc;
+    Expr *AllocateAlignment = nullptr;
   };
 
   OMPClause *ActOnOpenMPVarListClause(OpenMPClauseKind Kind,
@@ -1166,10 +1171,15 @@ class SemaOpenMP : public SemaBase {
                                         SourceLocation LParenLoc,
                                         SourceLocation EndLoc);
   /// Called on well-formed 'allocate' clause.
-  OMPClause *ActOnOpenMPAllocateClause(
-      Expr *Allocator, OpenMPAllocateClauseModifier ACModifier,
-      ArrayRef<Expr *> VarList, SourceLocation StartLoc,
-      SourceLocation ColonLoc, SourceLocation LParenLoc, SourceLocation EndLoc);
+  OMPClause *
+  ActOnOpenMPAllocateClause(Expr *Allocator, Expr *Alignment,
+                            OpenMPAllocateClauseModifier FirstModifier,
+                            SourceLocation FirstModifierLoc,
+                            OpenMPAllocateClauseModifier SecondModifier,
+                            SourceLocation SecondModifierLoc,
+                            ArrayRef<Expr *> VarList, SourceLocation StartLoc,
+                            SourceLocation ColonLoc, SourceLocation LParenLoc,
+                            SourceLocation EndLoc);
   /// Called on well-formed 'private' clause.
   OMPClause *ActOnOpenMPPrivateClause(ArrayRef<Expr *> VarList,
                                       SourceLocation StartLoc,
diff --git a/clang/lib/AST/OpenMPClause.cpp b/clang/lib/AST/OpenMPClause.cpp
index 4246ba95d827f1..532933d6183ce7 100644
--- a/clang/lib/AST/OpenMPClause.cpp
+++ b/clang/lib/AST/OpenMPClause.cpp
@@ -1019,19 +1019,18 @@ OMPPartialClause *OMPPartialClause::CreateEmpty(const ASTContext &C) {
   return new (C) OMPPartialClause();
 }
 
-OMPAllocateClause *
-OMPAllocateClause::Create(const ASTContext &C, SourceLocation StartLoc,
-                          SourceLocation LParenLoc, Expr *Allocator,
-                          SourceLocation ColonLoc,
-                          OpenMPAllocateClauseModifier AllocatorModifier,
-                          SourceLocation AllocatorModifierLoc,
-                          SourceLocation EndLoc, ArrayRef<Expr *> VL) {
+OMPAllocateClause *OMPAllocateClause::Create(
+    const ASTContext &C, SourceLocation StartLoc, SourceLocation LParenLoc,
+    Expr *Allocator, Expr *Alignment, SourceLocation ColonLoc,
+    OpenMPAllocateClauseModifier Modifier1, SourceLocation Modifier1Loc,
+    OpenMPAllocateClauseModifier Modifier2, SourceLocation Modifier2Loc,
+    SourceLocation EndLoc, ArrayRef<Expr *> VL) {
 
   // Allocate space for private variables and initializer expressions.
   void *Mem = C.Allocate(totalSizeToAlloc<Expr *>(VL.size()));
   auto *Clause = new (Mem) OMPAllocateClause(
-      StartLoc, LParenLoc, Allocator, ColonLoc, AllocatorModifier,
-      AllocatorModifierLoc, EndLoc, VL.size());
+      StartLoc, LParenLoc, Allocator, Alignment, ColonLoc, Modifier1,
+      Modifier1Loc, Modifier2, Modifier2Loc, EndLoc, VL.size());
 
   Clause->setVarRefs(VL);
   return Clause;
@@ -2245,21 +2244,48 @@ void OMPClausePrinter::VisitOMPClauseList(T *Node, char StartSym) {
 void OMPClausePrinter::VisitOMPAllocateClause(OMPAllocateClause *Node) {
   if (Node->varlist_empty())
     return;
+
+  Expr *FirstModifier = nullptr;
+  Expr *SecondModifier = nullptr;
+  auto FirstAllocMod = Node->getFirstAllocateModifier();
+  auto SecondAllocMod = Node->getSecondAllocateModifier();
+  bool FirstUnknown = FirstAllocMod == OMPC_ALLOCATE_unknown;
+  bool SecondUnknown = SecondAllocMod == OMPC_ALLOCATE_unknown;
+  if (FirstAllocMod == OMPC_ALLOCATE_allocator ||
+      (FirstAllocMod == OMPC_ALLOCATE_unknown && Node->getAllocator())) {
+    FirstModifier = Node->getAllocator();
+    SecondModifier = Node->getAlignment();
+  } else {
+    FirstModifier = Node->getAlignment();
+    SecondModifier = Node->getAllocator();
+  }
+
   OS << "allocate";
-  OpenMPAllocateClauseModifier Modifier = Node->getAllocatorModifier();
-  if (Expr *Allocator = Node->getAllocator()) {
+  // If we have any explicit modifiers.
+  if (FirstModifier) {
     OS << "(";
-    if (Modifier == OMPC_ALLOCATE_allocator) {
-      OS << getOpenMPSimpleClauseTypeName(Node->getClauseKind(), Modifier);
+    if (!FirstUnknown) {
+      OS << getOpenMPSimpleClauseTypeName(Node->getClauseKind(), FirstAllocMod);
       OS << "(";
-      Allocator->printPretty(OS, nullptr, Policy, 0);
+    }
+    FirstModifier->printPretty(OS, nullptr, Policy, 0);
+    if (!FirstUnknown)
       OS << ")";
-    } else {
-      Allocator->printPretty(OS, nullptr, Policy, 0);
+    if (SecondModifier) {
+      OS << ", ";
+      if (!SecondUnknown) {
+        OS << getOpenMPSimpleClauseTypeName(Node->getClauseKind(),
+                                            SecondAllocMod);
+        OS << "(";
+      }
+      SecondModifier->printPretty(OS, nullptr, Policy, 0);
+      if (!SecondUnknown)
+        OS << ")";
     }
     OS << ":";
     VisitOMPClauseList(Node, ' ');
   } else {
+    // No modifiers. Just print the variable list.
     VisitOMPClauseList(Node, '(');
   }
   OS << ")";
diff --git a/clang/lib/Parse/ParseOpenMP.cpp b/clang/lib/Parse/ParseOpenMP.cpp
index b4e973bc84a7b0..4032cbcb614e73 100644
--- a/clang/lib/Parse/ParseOpenMP.cpp
+++ b/clang/lib/Parse/ParseOpenMP.cpp
@@ -4530,32 +4530,87 @@ static bool parseStepSize(Parser &P, SemaOpenMP::OpenMPVarListDataTy &Data,
 }
 
 /// Parse 'allocate' clause modifiers.
-///   If allocator-modifier exists, return an expression for it and set
-///   Data field noting modifier was specified.
-///
+///   If allocator-modifier exists, return an expression for it. For both
+///   allocator and align modifiers, set Data fields as appropriate.
 static ExprResult
 parseOpenMPAllocateClauseModifiers(Parser &P, OpenMPClauseKind Kind,
                                    SemaOpenMP::OpenMPVarListDataTy &Data) {
   const Token &Tok = P.getCurToken();
   Preprocessor &PP = P.getPreprocessor();
   ExprResult Tail;
-  auto Modifier = static_cast<OpenMPAllocateClauseModifier>(
+  ExprResult Val;
+  SourceLocation RLoc;
+  bool AllocatorSeen = false;
+  bool AlignSeen = false;
+  SourceLocation CurrentModifierLoc = Tok.getLocation();
+  auto CurrentModifier = static_cast<OpenMPAllocateClauseModifier>(
       getOpenMPSimpleClauseType(Kind, PP.getSpelling(Tok), P.getLangOpts()));
-  if (Modifier == OMPC_ALLOCATE_allocator) {
-    Data.AllocClauseModifier = Modifier;
+
+  // Modifiers did not exist before 5.1
+  if (P.getLangOpts().OpenMP < 51)
+    return P.ParseAssignmentExpression();
+
+  // An allocator-simple-modifier is exclusive and must appear alone. See
+  // OpenMP6.0 spec, pg. 313, L1 on Modifiers, as well as Table 5.1, pg. 50,
+  // description of "exclusive" property. If we don't recognized an explicit
+  // simple-/complex- modifier, assume we're looking at expression
+  // representing allocator and consider ourselves done.
+  if (CurrentModifier == OMPC_ALLOCATE_unknown)
+    return P.ParseAssignmentExpression();
+
+  do {
     P.ConsumeToken();
-    BalancedDelimiterTracker AllocateT(P, tok::l_paren,
-                                       tok::annot_pragma_openmp_end);
     if (Tok.is(tok::l_paren)) {
-      AllocateT.consumeOpen();
-      Tail = P.ParseAssignmentExpression();
-      AllocateT.consumeClose();
+      switch (CurrentModifier) {
+      case OMPC_ALLOCATE_allocator: {
+        if (AllocatorSeen) {
+          P.Diag(Tok, diag::err_omp_duplicate_modifier)
+              << getOpenMPSimpleClauseTypeName(OMPC_allocate, CurrentModifier)
+              << getOpenMPClauseName(Kind);
+        } else {
+          Data.AllocClauseModifiers.push_back(CurrentModifier);
+          Data.AllocClauseModifiersLoc.push_back(CurrentModifierLoc);
+        }
+        BalancedDelimiterTracker AllocateT(P, tok::l_paren,
+                                           tok::annot_pragma_openmp_end);
+        AllocateT.consumeOpen();
+        Tail = P.ParseAssignmentExpression();
+        AllocateT.consumeClose();
+        AllocatorSeen = true;
+      } break;
+      case OMPC_ALLOCATE_align: {
+        if (AlignSeen) {
+          P.Diag(Tok, diag::err_omp_duplicate_modifier)
+              << getOpenMPSimpleClauseTypeName(OMPC_allocate, CurrentModifier)
+              << getOpenMPClauseName(Kind);
+        } else {
+          Data.AllocClauseModifiers.push_back(CurrentModifier);
+          Data.AllocClauseModifiersLoc.push_back(CurrentModifierLoc);
+        }
+        Val = P.ParseOpenMPParensExpr(getOpenMPClauseName(Kind), RLoc);
+        if (Val.isUsable())
+          Data.AllocateAlignment = Val.get();
+        AlignSeen = true;
+      } break;
+      default:
+        assert(false && "Unexpected allocate modifier");
+        break;
+      }
     } else {
       P.Diag(Tok, diag::err_expected) << tok::l_paren;
     }
-  } else {
-    Tail = P.ParseAssignmentExpression();
-  }
+    if (Tok.isNot(tok::comma))
+      break;
+    P.ConsumeToken();
+    CurrentModifierLoc = Tok.getLocation();
+    CurrentModifier = static_cast<OpenMPAllocateClauseModifier>(
+        getOpenMPSimpleClauseType(Kind, PP.getSpelling(Tok), P.getLangOpts()));
+    // A modifier followed by a comma implies another modifier.
+    if (CurrentModifier == OMPC_ALLOCATE_unknown) {
+      P.Diag(Tok, diag::err_omp_expected_modifier) << getOpenMPClauseName(Kind);
+      break;
+    }
+  } while (!AllocatorSeen || !AlignSeen);
   return Tail;
 }
 
@@ -4832,7 +4887,8 @@ bool Parser::ParseOpenMPVarList(OpenMPDirectiveKind DKind,
   } else if (Kind == OMPC_allocate ||
              (Kind == OMPC_affinity && Tok.is(tok::identifier) &&
               PP.getSpelling(Tok) == "iterator")) {
-    // Handle optional allocator expression followed by colon delimiter.
+    // Handle optional allocator and align modifiers followed by colon
+    // delimiter.
     ColonProtectionRAIIObject ColonRAII(*this);
     TentativeParsingAction TPA(*this);
     // OpenMP 5.0, 2.10.1, task Construct.
@@ -4849,19 +4905,18 @@ bool Parser::ParseOpenMPVarList(OpenMPDirectiveKind DKind,
     Tail = Actions.CorrectDelayedTyposInExpr(Tail);
     Tail = Actions.ActOnFinishFullExpr(Tail.get(), T.getOpenLocation(),
                                        /*DiscardedValue=*/false);
-    if (Tail.isUsable()) {
+    if (Tail.isUsable() || Data.AllocateAlignment) {
       if (Tok.is(tok::colon)) {
-        Data.DepModOrTailExpr = Tail.get();
+        Data.DepModOrTailExpr = Tail.isUsable() ? Tail.get() : nullptr;
         Data.ColonLoc = ConsumeToken();
         TPA.Commit();
       } else {
         // Colon not found, parse only list of variables.
         TPA.Revert();
-        if (Kind == OMPC_allocate &&
-            Data.AllocClauseModifier == OMPC_ALLOCATE_allocator) {
+        if (Kind == OMPC_allocate && Data.AllocClauseModifiers.size()) {
           SkipUntil(tok::r_paren, tok::annot_pragma_openmp_end,
                     StopBeforeMatch);
-          Diag(Tok, diag::err_modifier_expected_colon) << "allocator";
+          Diag(Tok, diag::err_modifier_expected_colon) << "allocate clause";
         }
       }
     } else {
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 66ff92f554fc42..9b77031ca16670 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -5285,6 +5285,7 @@ static void checkAllocateClauses(Sema &S, DSAStackTy ...
[truncated]

@ddpagan
Copy link
Contributor Author

ddpagan commented Jan 6, 2025

NOTE: OpenMP Support doc will be updated as well in a subsequent patch.

@alexey-bataev
Copy link
Member

Update OpenMPSupport.rst

@ddpagan
Copy link
Contributor Author

ddpagan commented Jan 6, 2025

Update OpenMPSupport.rst

New 'align' modifier added to implemented features list.

Tail = P.ParseAssignmentExpression();
AllocateT.consumeClose();
AllocatorSeen = true;
} break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} break;
break;}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and format

if (Val.isUsable())
Data.AllocateAlignment = Val.get();
AlignSeen = true;
} break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

@@ -5285,6 +5285,7 @@ static void checkAllocateClauses(Sema &S, DSAStackTy *Stack,
}
for (OMPClause *C : AllocateRange) {
auto *AC = cast<OMPAllocateClause>(C);
// TODO: Check alignment?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the missing checks here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old note to myself. As far as I know there aren't any checks required here.

@ddpagan
Copy link
Contributor Author

ddpagan commented Jan 7, 2025

Thanks for reviewing, Alexey. Made the requested changes.

@ddpagan ddpagan requested a review from alexey-bataev January 7, 2025 20:04
break;
}
default:
assert(false && "Unexpected allocate modifier");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert(false && "Unexpected allocate modifier");
lvm_unreachable("Unexpected allocate modifier");

Comment on lines 7927 to 7928
Record.push_back(C->getFirstAllocateModifier());
Record.push_back(C->getSecondAllocateModifier());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

writeEnum?

Comment on lines 11828 to 11830
static_cast<OpenMPAllocateClauseModifier>(Record.readInt()));
C->setSecondAllocateModifier(
static_cast<OpenMPAllocateClauseModifier>(Record.readInt()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not readEnum?

@ddpagan
Copy link
Contributor Author

ddpagan commented Jan 9, 2025

Made changes requested by Alexey.

@ddpagan ddpagan requested a review from alexey-bataev January 9, 2025 00:29
OpenMPAllocateClauseModifier Modifier1 = OMPC_ALLOCATE_unknown;
OpenMPAllocateClauseModifier Modifier2 = OMPC_ALLOCATE_unknown;
SourceLocation Modifier1Loc, Modifier2Loc;
if (auto NumModifiers = Data.AllocClauseModifiers.size()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (auto NumModifiers = Data.AllocClauseModifiers.size()) {
if (!Data.AllocClauseModifiers.empty()) {

@ddpagan ddpagan requested a review from alexey-bataev January 9, 2025 16:52
Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG with a nit

}
default:
llvm_unreachable("Unexpected allocate modifier");
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for break here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reviews, Alexey. I'll take care of this last issue.

@ddpagan ddpagan requested a review from alexey-bataev January 9, 2025 21:55
@alexey-bataev
Copy link
Member

Rebase? And commit after rebase, it is approved already

The 'align' modifier is now accepted in the 'allocate' clause. Added
LIT tests covering codegen, PCH, template handling, and serialization
for 'align' modifier.

Added support for align-modifier to release notes.

Testing
- New allocate modifier LIT tests.
- OpenMP LIT tests.
- check-all
For serialization, use writeEnum/readEnum for enum modifier values.
@ddpagan ddpagan merged commit ad38e24 into llvm:main Jan 13, 2025
9 checks passed
kazutakahirata pushed a commit to kazutakahirata/llvm-project that referenced this pull request Jan 13, 2025
The 'align' modifier is now accepted in the 'allocate' clause. Added LIT
tests covering codegen, PCH, template handling, and serialization for
'align' modifier.

Added support for align-modifier to release notes.

Testing
- New allocate modifier LIT tests.
- OpenMP LIT tests.
- check-all
@ddpagan ddpagan deleted the align-modifier branch January 24, 2025 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants