[NFC] Factor out common parts of ArraySections into its own class #89639

erichkeane · 2024-04-22T17:54:05Z

OpenACC is going to need an array sections implementation that is a simpler version/more restrictive version of the OpenMP version. This patch extracts the things that should be common between the two into a template base class. This will allow a followup patch to implement an OpenACC array sections expression type.

llvmbot · 2024-04-22T17:54:37Z

@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

OpenACC is going to need an array sections implementation that is a simpler version/more restrictive version of the OpenMP version. This patch extracts the things that should be common between the two into a template base class. This will allow a followup patch to implement an OpenACC array sections expression type.

Full diff: https://github.com/llvm/llvm-project/pull/89639.diff

2 Files Affected:

(modified) clang/include/clang/AST/Expr.h (+105)
(modified) clang/include/clang/AST/ExprOpenMP.h (+14-60)

diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index 2bfefeabc348be..3fc0170d1a0d8e 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -6610,6 +6610,111 @@ class TypoExpr : public Expr {
 
 };
 
+// This is a sub-class for OpenMP and OpenACC array sections. OpenACC uses a
+// very small subset of the functionality, so this class only exposes the things
+// the two have in common. This type is not expected to be used directly,
+// instead it is inherited from by the OMP and OpenACC variants.
+//
+// This type does not capture a 'stride', only a lower-bound and length (plus
+// the base expression), but provides room via a template argument to get
+// additional ones.
+template<unsigned NumSubExprs, bool AllowNullExprs>
+class ArraySectionExprBase : public Expr {
+  enum {BASE, LOWER_BOUND, LENGTH, END_EXPR = NumSubExprs};
+  Stmt *SubExprs[END_EXPR];
+  SourceLocation ColonLocFirst;
+  SourceLocation RBracketLoc;
+
+  protected:
+    ArraySectionExprBase(StmtClass SC, Expr *Base, Expr *LowerBound,
+                         Expr *Length, QualType Type, ExprValueKind VK,
+                         ExprObjectKind OK, SourceLocation ColonLocFirst,
+                         SourceLocation RBracketLoc)
+        : Expr(SC, Type, VK, OK), ColonLocFirst(ColonLocFirst),
+          RBracketLoc(RBracketLoc) {
+      setBase(Base);
+      setLowerBound(LowerBound);
+      setLength(Length);
+    }
+
+    explicit ArraySectionExprBase(StmtClass SC, EmptyShell Shell)
+        : Expr(SC, Shell) {}
+
+    void setSubExpr(unsigned Idx, Expr *SubExpr) {
+      assert(Idx > LENGTH &&
+             "setting sub expression owned by ArraySectionExprBase: Should be "
+             "using the direct 'setter' functions");
+      assert((SubExpr || AllowNullExprs) && "Null expression when not allowed");
+      SubExprs[Idx] = SubExpr;
+    }
+
+    Expr *getSubExpr(unsigned Idx) {
+      assert(Idx > LENGTH &&
+             "getting sub expression owned by ArraySectionExprBase: Should be "
+             "using the direct 'getter' functions");
+      return cast_or_null<Expr>(SubExprs[Idx]);
+    }
+    const Expr *getSubExpr(unsigned Idx) const {
+      assert(Idx > LENGTH &&
+             "getting sub expression owned by ArraySectionExprBase: Should be "
+             "using the direct 'getter' functions");
+      return cast_or_null<Expr>(SubExprs[Idx]);
+    }
+
+  public:
+  /// Get base of the array section.
+  Expr *getBase() { return cast<Expr>(SubExprs[BASE]); }
+  const Expr *getBase() const { return cast<Expr>(SubExprs[BASE]); }
+  /// Set base of the array section.
+  void setBase(Expr *E) {
+    assert((E || AllowNullExprs) && "Null expression when not allowed");
+    SubExprs[BASE] = E;
+  }
+
+  /// Get lower bound of array section.
+  Expr *getLowerBound() { return cast_or_null<Expr>(SubExprs[LOWER_BOUND]); }
+  const Expr *getLowerBound() const {
+    return cast_or_null<Expr>(SubExprs[LOWER_BOUND]);
+  }
+  /// Set lower bound of the array section.
+  void setLowerBound(Expr *E) {
+    assert((E || AllowNullExprs) && "Null expression when not allowed");
+    SubExprs[LOWER_BOUND] = E;
+  }
+
+  /// Get length of array section.
+  Expr *getLength() { return cast_or_null<Expr>(SubExprs[LENGTH]); }
+  const Expr *getLength() const { return cast_or_null<Expr>(SubExprs[LENGTH]); }
+  /// Set length of the array section.
+  void setLength(Expr *E) {
+    assert((E || AllowNullExprs) && "Null expression when not allowed");
+    SubExprs[LENGTH] = E;
+  }
+
+  SourceLocation getBeginLoc() const LLVM_READONLY {
+    return getBase()->getBeginLoc();
+  }
+
+  SourceLocation getEndLoc() const LLVM_READONLY { return RBracketLoc; }
+
+  SourceLocation getColonLocFirst() const { return ColonLocFirst; }
+  void setColonLocFirst(SourceLocation L) { ColonLocFirst = L; }
+
+  SourceLocation getRBracketLoc() const { return RBracketLoc; }
+  void setRBracketLoc(SourceLocation L) { RBracketLoc = L; }
+
+  SourceLocation getExprLoc() const LLVM_READONLY {
+    return getBase()->getExprLoc();
+  }
+    child_range children() {
+      return child_range(&SubExprs[BASE], &SubExprs[END_EXPR]);
+    }
+
+    const_child_range children() const {
+      return const_child_range(&SubExprs[BASE], &SubExprs[END_EXPR]);
+    }
+};
+
 /// Frontend produces RecoveryExprs on semantic errors that prevent creating
 /// other well-formed expressions. E.g. when type-checking of a binary operator
 /// fails, we cannot produce a BinaryOperator expression. Instead, we can choose
diff --git a/clang/include/clang/AST/ExprOpenMP.h b/clang/include/clang/AST/ExprOpenMP.h
index be5b1f3fdd112f..c410c82d9469a3 100644
--- a/clang/include/clang/AST/ExprOpenMP.h
+++ b/clang/include/clang/AST/ExprOpenMP.h
@@ -53,92 +53,46 @@ namespace clang {
 /// When the length is absent it defaults to ⌈(size − lower-bound)/stride⌉,
 /// where size is the size of the array dimension. When the lower-bound is
 /// absent it defaults to 0.
-class OMPArraySectionExpr : public Expr {
+namespace OMPArraySectionIndices {
   enum { BASE, LOWER_BOUND, LENGTH, STRIDE, END_EXPR };
-  Stmt *SubExprs[END_EXPR];
-  SourceLocation ColonLocFirst;
+}
+class OMPArraySectionExpr
+    : public ArraySectionExprBase<OMPArraySectionIndices::END_EXPR,
+                                  /*AllowNullExprs=*/true> {
   SourceLocation ColonLocSecond;
-  SourceLocation RBracketLoc;
 
 public:
   OMPArraySectionExpr(Expr *Base, Expr *LowerBound, Expr *Length, Expr *Stride,
                       QualType Type, ExprValueKind VK, ExprObjectKind OK,
                       SourceLocation ColonLocFirst,
                       SourceLocation ColonLocSecond, SourceLocation RBracketLoc)
-      : Expr(OMPArraySectionExprClass, Type, VK, OK),
-        ColonLocFirst(ColonLocFirst), ColonLocSecond(ColonLocSecond),
-        RBracketLoc(RBracketLoc) {
-    SubExprs[BASE] = Base;
-    SubExprs[LOWER_BOUND] = LowerBound;
-    SubExprs[LENGTH] = Length;
-    SubExprs[STRIDE] = Stride;
+      : ArraySectionExprBase(OMPArraySectionExprClass, Base, LowerBound, Length,
+                             Type, VK, OK, ColonLocFirst, RBracketLoc) {
+    setSubExpr(OMPArraySectionIndices::STRIDE, Stride);
     setDependence(computeDependence(this));
   }
 
   /// Create an empty array section expression.
   explicit OMPArraySectionExpr(EmptyShell Shell)
-      : Expr(OMPArraySectionExprClass, Shell) {}
-
-  /// An array section can be written only as Base[LowerBound:Length].
-
-  /// Get base of the array section.
-  Expr *getBase() { return cast<Expr>(SubExprs[BASE]); }
-  const Expr *getBase() const { return cast<Expr>(SubExprs[BASE]); }
-  /// Set base of the array section.
-  void setBase(Expr *E) { SubExprs[BASE] = E; }
+      : ArraySectionExprBase(OMPArraySectionExprClass, Shell) {}
 
   /// Return original type of the base expression for array section.
   static QualType getBaseOriginalType(const Expr *Base);
 
-  /// Get lower bound of array section.
-  Expr *getLowerBound() { return cast_or_null<Expr>(SubExprs[LOWER_BOUND]); }
-  const Expr *getLowerBound() const {
-    return cast_or_null<Expr>(SubExprs[LOWER_BOUND]);
-  }
-  /// Set lower bound of the array section.
-  void setLowerBound(Expr *E) { SubExprs[LOWER_BOUND] = E; }
-
-  /// Get length of array section.
-  Expr *getLength() { return cast_or_null<Expr>(SubExprs[LENGTH]); }
-  const Expr *getLength() const { return cast_or_null<Expr>(SubExprs[LENGTH]); }
-  /// Set length of the array section.
-  void setLength(Expr *E) { SubExprs[LENGTH] = E; }
-
   /// Get stride of array section.
-  Expr *getStride() { return cast_or_null<Expr>(SubExprs[STRIDE]); }
-  const Expr *getStride() const { return cast_or_null<Expr>(SubExprs[STRIDE]); }
-  /// Set length of the array section.
-  void setStride(Expr *E) { SubExprs[STRIDE] = E; }
-
-  SourceLocation getBeginLoc() const LLVM_READONLY {
-    return getBase()->getBeginLoc();
+  Expr *getStride() { return getSubExpr(OMPArraySectionIndices::STRIDE); }
+  const Expr *getStride() const {
+    return getSubExpr(OMPArraySectionIndices::STRIDE);
   }
-  SourceLocation getEndLoc() const LLVM_READONLY { return RBracketLoc; }
-
-  SourceLocation getColonLocFirst() const { return ColonLocFirst; }
-  void setColonLocFirst(SourceLocation L) { ColonLocFirst = L; }
+  /// Set length of the array section.
+  void setStride(Expr *E) { setSubExpr(OMPArraySectionIndices::STRIDE, E); }
 
   SourceLocation getColonLocSecond() const { return ColonLocSecond; }
   void setColonLocSecond(SourceLocation L) { ColonLocSecond = L; }
 
-  SourceLocation getRBracketLoc() const { return RBracketLoc; }
-  void setRBracketLoc(SourceLocation L) { RBracketLoc = L; }
-
-  SourceLocation getExprLoc() const LLVM_READONLY {
-    return getBase()->getExprLoc();
-  }
-
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == OMPArraySectionExprClass;
   }
-
-  child_range children() {
-    return child_range(&SubExprs[BASE], &SubExprs[END_EXPR]);
-  }
-
-  const_child_range children() const {
-    return const_child_range(&SubExprs[BASE], &SubExprs[END_EXPR]);
-  }
 };
 
 /// An explicit cast in C or a C-style cast in C++, which uses the syntax

erichkeane · 2024-04-22T17:55:50Z

Note: this is one of my approaches. This ends up touching the OpenMP work 'the least' (in that this is the entirety of the OpenMP changes as best I can tell), at the expense of more work/a separate AST Node for OpenACC.

The OTHER approach I have is to move/rename OpenMPArraySectionExpr to just ArraySectionExpr, and give it an IsOpenACC or IsOpenMP type function, and change everywhere it is used to check/assert on that.

I'm totally open to other ideas though, I'd love to have a good solution for this.

github-actions · 2024-04-22T17:56:50Z

✅ With the latest revision this PR passed the C/C++ code formatter.

alexey-bataev · 2024-04-22T17:57:19Z

clang/include/clang/AST/Expr.h

+  /// Set base of the array section.
+  void setBase(Expr *E) {
+    assert((E || AllowNullExprs) && "Null expression when not allowed");
+    SubExprs[BASE] = E;
+  }
+


Can you make it private or protected?

I made it protected (since I want the constructors of the inheritors to use it) and had to make ASTStmtReader a friend.

alexey-bataev · 2024-04-22T17:57:26Z

clang/include/clang/AST/Expr.h

+  /// Set lower bound of the array section.
+  void setLowerBound(Expr *E) {
+    assert((E || AllowNullExprs) && "Null expression when not allowed");
+    SubExprs[LOWER_BOUND] = E;
+  }


alexey-bataev · 2024-04-22T17:57:34Z

clang/include/clang/AST/Expr.h

+  /// Set length of the array section.
+  void setLength(Expr *E) {
+    assert((E || AllowNullExprs) && "Null expression when not allowed");
+    SubExprs[LENGTH] = E;
+  }


alexey-bataev

LG

erichkeane · 2024-04-23T14:23:30Z

@njames93 : My test failure has to do with the "NodeIntrospection" stuff that you seemed to review ~3 years ago: https://reviews.llvm.org/D93164

At the moment, I cannot even figure out how to get it to build, but I suspect it is getting confused in some way by having a non-AST-node base class of a type? Any help you could give would be appreciated.

erichkeane · 2024-04-23T15:13:11Z

@alexey-bataev
Not having this new 'base' type as a member of the StmtNodes.td results in one of the Clang tools failing (see the CI). AND, since it is a template, it can't do that.

I can't make it a Mixin, because it needs to provide some of the Expr-type functions regarding source location/etc.

So I don't have a great idea on how to have reuse between the two while still having them be separate AST nodes (as there IS a bit of a difference in behaviors).

I COULD just move the OMP type to Expr.h (renamed as just ArraySectionExpr), add a flag for OMP vs OpenACC, then make the logic of each function check it, but I'm tentative as to whether that is better.

alexey-bataev · 2024-04-23T15:18:54Z

@alexey-bataev Not having this new 'base' type as a member of the StmtNodes.td results in one of the Clang tools failing (see the CI). AND, since it is a template, it can't do that.

I can't make it a Mixin, because it needs to provide some of the Expr-type functions regarding source location/etc.

So I don't have a great idea on how to have reuse between the two while still having them be separate AST nodes (as there IS a bit of a difference in behaviors).

I COULD just move the OMP type to Expr.h (renamed as just ArraySectionExpr), add a flag for OMP vs OpenACC, then make the logic of each function check it, but I'm tentative as to whether that is better.

I'm fine with having just a single AST node

erichkeane · 2024-04-23T15:24:05Z

@alexey-bataev Not having this new 'base' type as a member of the StmtNodes.td results in one of the Clang tools failing (see the CI). AND, since it is a template, it can't do that.
I can't make it a Mixin, because it needs to provide some of the Expr-type functions regarding source location/etc.
So I don't have a great idea on how to have reuse between the two while still having them be separate AST nodes (as there IS a bit of a difference in behaviors).
I COULD just move the OMP type to Expr.h (renamed as just ArraySectionExpr), add a flag for OMP vs OpenACC, then make the logic of each function check it, but I'm tentative as to whether that is better.

I'm fine with having just a single AST node

Ok, I'll start on the work to make this happen.

This version of the patch generializes the OMPArraySectionExpr to be more generic, and renames it ArraySectionExpr. There is a LOT of work involved obviously, but it is pretty rote. I've also added stubs for the OpenACC implementation/use of this, but there is currently no Sema for it implemented. Note that I've found a few small 'bugs' in the OpenMP implementation while auditing hte code, which I'll mention inline.

erichkeane

I opted to leave the type of both array sections the same as it seems like it will save a bunch of work and complexity, at the expense of 1 'print', and potentially 1 diagnostic. Let me know if you think this is an important distinction!

erichkeane · 2024-04-24T14:59:50Z

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -11163,7 +11163,7 @@ def err_omp_declare_mapper_redefinition : Error<
  "redefinition of user-defined mapper for type %0 with name %1">;
 def err_omp_invalid_mapper: Error<
  "cannot find a valid user-defined mapper for type %0 with name %1">;
-def err_omp_array_section_use : Error<"OpenMP array section is not allowed here">;
+def err_array_section_use : Error<"array section is not allowed here">;


I generalized this diagnostic as well, though I see I could PERHAPS get this right based on the type of the Expression above. WDYT?

Would be good to select proper extension type here in the diagnostic, at least for OpenMP

Alright, will do!

erichkeane · 2024-04-24T15:00:20Z

clang/lib/AST/ComputeDependence.cpp

  auto D = E->getBase()->getDependence();
  if (auto *LB = E->getLowerBound())
    D |= LB->getDependence();
  if (auto *Len = E->getLength())
    D |= Len->getDependence();
+
+  if (E->isOMPArraySection()) {


Note that this is a 'bug fix' here, we weren't computing dependence based on stride.

Nice catch.

erichkeane · 2024-04-24T15:00:52Z

clang/lib/AST/Type.cpp

-  case OMPArraySection:
-    return "<OpenMP array section type>";
+  case ArraySection:
+    return "<array section type>";


Generalized again, but this is one I couldn't really do anything about, all I have is the 'type', not the associated expression.

erichkeane · 2024-04-24T15:03:38Z

clang/lib/Sema/TreeTransform.h

-      LowerBound.get() == E->getLowerBound() && Length.get() == E->getLength())
+      LowerBound.get() == E->getLowerBound() &&
+      Length.get() == E->getLength() &&
+      (E->isOpenACCArraySection() || Stride.get() == E->getStride()))


This is another bug fix, we previously weren't checking this correctly here.

clang/include/clang/AST/Expr.h

… error message

erichkeane · 2024-04-25T13:19:38Z

I think I've got everything done you were concerned about. Let me know if you see anything else!

alexey-bataev

LG

NagyDonat · 2024-04-26T11:08:31Z

clang/test/ParserOpenACC/parse-cache-construct.cpp

    // FIXME: Once we have a new array-section type to represent OpenACC as
    // well, change this error message.


This FIXME was resolved by this commit, consider removing it in a trivial follow-up change.

Thanks, done in bb1a8bb

erichkeane requested a review from alexey-bataev April 22, 2024 17:54

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels Apr 22, 2024

alexey-bataev reviewed Apr 22, 2024

View reviewed changes

Move to protected + clang-format

e473961

alexey-bataev approved these changes Apr 22, 2024

View reviewed changes

Correctly initialize second-colon-loc so it prints correctly

c8a9094

llvmbot added clang:modules C++20 modules and Clang Header Modules clang:codegen IR generation bugs: mangling, exceptions, etc. clang:static analyzer labels Apr 24, 2024

erichkeane commented Apr 24, 2024

View reviewed changes

Clang-format

40e7dea

alexey-bataev reviewed Apr 24, 2024

View reviewed changes

clang/include/clang/AST/Expr.h Outdated Show resolved Hide resolved

clang/include/clang/AST/Expr.h Outdated Show resolved Hide resolved

clang/include/clang/AST/Expr.h Outdated Show resolved Hide resolved

erichkeane added 2 commits April 24, 2024 11:49

Do the requested changes to ArraySectionExpr, be more specific on the…

883b556

… error message

Clang-format

bac62f3

alexey-bataev approved these changes Apr 25, 2024

View reviewed changes

erichkeane merged commit 39adc8f into llvm:main Apr 25, 2024
5 of 6 checks passed

NagyDonat reviewed Apr 26, 2024

View reviewed changes

		// FIXME: Once we have a new array-section type to represent OpenACC as
		// well, change this error message.

[NFC] Factor out common parts of ArraySections into its own class #89639

[NFC] Factor out common parts of ArraySections into its own class #89639

Uh oh!

Conversation

erichkeane commented Apr 22, 2024

Uh oh!

llvmbot commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane commented Apr 22, 2024

Uh oh!

github-actions bot commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-bataev left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Apr 23, 2024

Uh oh!

erichkeane commented Apr 23, 2024

Uh oh!

alexey-bataev commented Apr 23, 2024

Uh oh!

erichkeane commented Apr 23, 2024

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane commented Apr 25, 2024

Uh oh!

alexey-bataev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Apr 22, 2024 •

edited

Loading

github-actions bot commented Apr 22, 2024 •

edited

Loading