[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

https://github.com/Meinersbur closed 
https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

https://github.com/Meinersbur edited 
https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

https://github.com/Meinersbur edited 
https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

https://github.com/Meinersbur edited 
https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

https://github.com/alexey-bataev approved this pull request.

LG

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

@@ -156,9 +156,9 @@ extern "C" void body(...) {}
 // IR-EMPTY:
 // IR-NEXT:  [[FOR_INC]]:
 // IR-NEXT:%[[TMP34:.+]] = load i32, ptr %[[DOTTILE_0_IV_I]], align 4
-// IR-NEXT:%[[INC:.+]] = add nsw i32 %[[TMP34]], 1
+// IR-NEXT:%[[INC:.+]] = add i32 %[[TMP34]], 1

Meinersbur wrote:

`nuw` would be an idea for a Clang extension in the footsteps of [vector 
extensions](https://releases.llvm.org/18.1.1/tools/clang/docs/LanguageExtensions.html#vectors-and-extended-vectors)
 or 
[`_ExtInt`](http://blog.llvm.org/2020/04/the-new-clang-extint-feature-provides.html)
 which also exposed a feature that LLVM always supported. E.g. 
```
typedef unsigned nowrap_unsigned_t __attribute__((ext_no_wrap));
for (nowrap_unsigned_t i = 0; i < n; ++i)
```
or
```
int [[clang::]]
```
However, the language consequences are immense.

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

@@ -156,9 +156,9 @@ extern "C" void body(...) {}
 // IR-EMPTY:
 // IR-NEXT:  [[FOR_INC]]:
 // IR-NEXT:%[[TMP34:.+]] = load i32, ptr %[[DOTTILE_0_IV_I]], align 4
-// IR-NEXT:%[[INC:.+]] = add nsw i32 %[[TMP34]], 1
+// IR-NEXT:%[[INC:.+]] = add i32 %[[TMP34]], 1

Meinersbur wrote:

This is due to the change of using the type of `LoopHelper.NumIterations` (an 
unsigned integer) as the new loop's induction variable (instead of the original 
loop's iteration variable type). In iterator-based loops that type would be 
incompatible with `LoopHelper.NumIterations` which remains an integer.

Even if the original loop variable is a signed integer (it could be a 
(`std::iota_view`)[https://en.cppreference.com/w/cpp/ranges/iota_view] over the 
same range), using and unsigned type is more correct as long as we start 
counting at 0. For instance, the loop
```
#pragma omp tile sizes(2)
for (int i = INT_MIN; i < INT_MAX; ++i)
```
would overflow the `nsw` flag.

The "most correct" fix would be to add the `nuw` flag. Unfortunately there is 
no AST expression we could create that would make CodeGen generate one. 
Alternatively, I could make generated loops start counting at `INT_MIN`.

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

@@ -15095,16 +15136,70 @@ bool SemaOpenMP::checkTransformableLoopNest(
   DependentPreInits = Dir->getPreInits();
 else
   llvm_unreachable("Unhandled loop transformation");
-if (!DependentPreInits)
-  return;
-llvm::append_range(OriginalInits.back(),
-   cast(DependentPreInits)->getDeclGroup());
+
+appendFlattendedStmtList(OriginalInits.back(), DependentPreInits);
   });
   assert(OriginalInits.back().empty() && "No preinit after innermost loop");
   OriginalInits.pop_back();
   return Result;
 }
 
+/// Add preinit statements that need to be propageted from the selected loop.
+static void addLoopPreInits(ASTContext &Context,
+OMPLoopBasedDirective::HelperExprs &LoopHelper,
+Stmt *LoopStmt, ArrayRef OriginalInit,
+SmallVectorImpl &PreInits) {
+
+  // For range-based for-statements, ensure that their syntactic sugar is
+  // executed by adding them as pre-init statements.
+  if (auto *CXXRangeFor = dyn_cast(LoopStmt)) {
+Stmt *RangeInit = CXXRangeFor->getInit();
+if (RangeInit)
+  PreInits.push_back(RangeInit);
+
+DeclStmt *RangeStmt = CXXRangeFor->getRangeStmt();
+PreInits.push_back(new (Context) DeclStmt(RangeStmt->getDeclGroup(),
+  RangeStmt->getBeginLoc(),
+  RangeStmt->getEndLoc()));
+
+DeclStmt *RangeEnd = CXXRangeFor->getEndStmt();
+PreInits.push_back(new (Context) DeclStmt(RangeEnd->getDeclGroup(),
+  RangeEnd->getBeginLoc(),
+  RangeEnd->getEndLoc()));
+  }
+
+  llvm::append_range(PreInits, OriginalInit);
+
+  // List of OMPCapturedExprDecl, for __begin, __end, and NumIterations
+  if (auto *PI = cast_or_null(LoopHelper.PreInits)) {
+PreInits.push_back(new (Context) DeclStmt(
+PI->getDeclGroup(), PI->getBeginLoc(), PI->getEndLoc()));
+  }
+
+  // Gather declarations for the data members used as counters.
+  for (Expr *CounterRef : LoopHelper.Counters) {
+auto *CounterDecl = cast(CounterRef)->getDecl();
+if (isa(CounterDecl))
+  PreInits.push_back(new (Context) DeclStmt(
+  DeclGroupRef(CounterDecl), SourceLocation(), SourceLocation()));
+  }
+}
+
+/// Collect the loop statements (ForStmt or CXXRangeForStmt) of the affected
+/// loop of a construct.
+static void collectLoopStmts(Stmt *AStmt, MutableArrayRef LoopStmts) {
+  size_t NumLoops = LoopStmts.size();
+  OMPLoopBasedDirective::doForAllLoops(
+  AStmt, /*TryImperfectlyNestedLoops=*/false, NumLoops,
+  [LoopStmts](unsigned Cnt, Stmt *CurStmt) {
+assert(!LoopStmts[Cnt] && "Loop statement must not yet be assigned");
+LoopStmts[Cnt] = CurStmt;
+return false;
+  });
+  assert(llvm::all_of(LoopStmts, [](Stmt *LoopStmt) { return LoopStmt; }) &&
+ "Expecting a loop statement for each affected loop");

alexey-bataev wrote:

```suggestion
  assert(!is_contained(LoopStmts, nullptr) &&
 "Expecting a loop statement for each affected loop");
```

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

@@ -9924,11 +9954,24 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr 
*CollapseLoopCountExpr,
 Stmt *DependentPreInits = Transform->getPreInits();
 if (!DependentPreInits)
   return;
-for (Decl *C : cast(DependentPreInits)->getDeclGroup()) {
-  auto *D = cast(C);
-  DeclRefExpr *Ref = buildDeclRefExpr(SemaRef, D, D->getType(),
-  Transform->getBeginLoc());
-  Captures[Ref] = Ref;
+
+// Search for pre-init declared variables that need to be captured
+// to be referenceable inside the directive.
+SmallVector Constituents;
+if (auto *CS = dyn_cast(DependentPreInits))
+  llvm::append_range(Constituents, CS->body());
+else
+  Constituents.push_back(DependentPreInits);

alexey-bataev wrote:

```suggestion
appendFlattendedStmtList(Constituents, DependentPreInits);
```

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

@@ -156,9 +156,9 @@ extern "C" void body(...) {}
 // IR-EMPTY:
 // IR-NEXT:  [[FOR_INC]]:
 // IR-NEXT:%[[TMP34:.+]] = load i32, ptr %[[DOTTILE_0_IV_I]], align 4
-// IR-NEXT:%[[INC:.+]] = add nsw i32 %[[TMP34]], 1
+// IR-NEXT:%[[INC:.+]] = add i32 %[[TMP34]], 1

alexey-bataev wrote:

Why nsw is dropped? Another question - can be nuw added?

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

@@ -9924,11 +9941,24 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr 
*CollapseLoopCountExpr,
 Stmt *DependentPreInits = Transform->getPreInits();
 if (!DependentPreInits)
   return;
-for (Decl *C : cast(DependentPreInits)->getDeclGroup()) {
-  auto *D = cast(C);
-  DeclRefExpr *Ref = buildDeclRefExpr(SemaRef, D, D->getType(),
-  Transform->getBeginLoc());
-  Captures[Ref] = Ref;
+
+// Search for pre-init declared variables that need to be captured
+// to be referenceable inside the directive.
+SmallVector Constituents;
+if (auto *CS = dyn_cast(DependentPreInits))
+  llvm::append_range(Constituents, CS->body());
+else
+  Constituents.push_back(DependentPreInits);
+for (Stmt *S : Constituents) {
+  if (DeclStmt *DC = dyn_cast(S)) {

alexey-bataev wrote:

```suggestion
  if (auto *DC = dyn_cast(S)) {
```

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

@@ -15097,14 +15125,75 @@ bool SemaOpenMP::checkTransformableLoopNest(
   llvm_unreachable("Unhandled loop transformation");
 if (!DependentPreInits)
   return;
-llvm::append_range(OriginalInits.back(),
-   cast(DependentPreInits)->getDeclGroup());
+// CompoundStmts are used as lists of other statements, add their
+// contents, not the lists themselves to avoid nesting. This is
+// necessary because DeclStmts need to be visible after the pre-init.
+else if (auto *CS = dyn_cast(DependentPreInits))
+  llvm::append_range(OriginalInits.back(), CS->body());
+else
+  OriginalInits.back().push_back(DependentPreInits);

alexey-bataev wrote:

Suggest moving this pattern to a function to avoid copying it.

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

@@ -182,17 +182,34 @@ class OMPLoopScope : public 
CodeGenFunction::RunCleanupsScope {
 }
 return false;
   });
-  PreInits = cast_or_null(LD->getPreInits());
+  PreInits = LD->getPreInits();
 } else if (const auto *Tile = dyn_cast(&S)) {
-  PreInits = cast_or_null(Tile->getPreInits());
+  PreInits = Tile->getPreInits();
 } else if (const auto *Unroll = dyn_cast(&S)) {
-  PreInits = cast_or_null(Unroll->getPreInits());
+  PreInits = Unroll->getPreInits();
 } else {
   llvm_unreachable("Unknown loop-based directive kind.");
 }
 if (PreInits) {
-  for (const auto *I : PreInits->decls())
-CGF.EmitVarDecl(cast(*I));
+  // CompoundStmts and DeclStmts are used as lists of PreInit statements 
and
+  // declarations. Since declarations must be visible in the the following
+  // that they initialize, unpack the ComboundStmt they are nested in.
+  SmallVector PreInitStmts;
+  if (auto *PreInitCompound = dyn_cast(PreInits))
+llvm::append_range(PreInitStmts, PreInitCompound->body());
+  else
+PreInitStmts.push_back(PreInits);
+
+  for (const Stmt *S : PreInitStmts) {
+// EmitStmt skips any OMPCapturedExprDecls, but needs to be emitted
+// here.
+if (auto *PreInitDecl = dyn_cast(S)) {

Meinersbur wrote:

Everything else is emitted in `CGF.EmitStmt(S);` at line 211.

`CGF.EmitStmt(S)` does itself call `CGF.EmitVarDecl(S)` if passed a DeclStmts, 
so this special handling should not be necessary. It includes, however, an 
exception for `OMPCapturedExprDecl` (subclass of `VarDecl`) that are NOT 
emitted so we need to do this explicitly here. Otherwise, lines 203-212 would 
be just a single `CGF.EmitStmt(S)`.

https://github.com/llvm/llvm-project/blob/a15b685c2d868eaf408d05baa50baa3c9f5cc740/clang/lib/CodeGen/CGDecl.cpp#L129

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Alexey Bataev via cfe-commits]

@@ -182,17 +182,34 @@ class OMPLoopScope : public 
CodeGenFunction::RunCleanupsScope {
 }
 return false;
   });
-  PreInits = cast_or_null(LD->getPreInits());
+  PreInits = LD->getPreInits();
 } else if (const auto *Tile = dyn_cast(&S)) {
-  PreInits = cast_or_null(Tile->getPreInits());
+  PreInits = Tile->getPreInits();
 } else if (const auto *Unroll = dyn_cast(&S)) {
-  PreInits = cast_or_null(Unroll->getPreInits());
+  PreInits = Unroll->getPreInits();
 } else {
   llvm_unreachable("Unknown loop-based directive kind.");
 }
 if (PreInits) {
-  for (const auto *I : PreInits->decls())
-CGF.EmitVarDecl(cast(*I));
+  // CompoundStmts and DeclStmts are used as lists of PreInit statements 
and
+  // declarations. Since declarations must be visible in the the following
+  // that they initialize, unpack the ComboundStmt they are nested in.
+  SmallVector PreInitStmts;
+  if (auto *PreInitCompound = dyn_cast(PreInits))
+llvm::append_range(PreInitStmts, PreInitCompound->body());
+  else
+PreInitStmts.push_back(PreInits);
+
+  for (const Stmt *S : PreInitStmts) {
+// EmitStmt skips any OMPCapturedExprDecls, but needs to be emitted
+// here.
+if (auto *PreInitDecl = dyn_cast(S)) {

alexey-bataev wrote:

So, you still emit only DeclStmts? Then why you can't put all Decls into a 
single one DeclStmt upon creation in Sema? 

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

@@ -182,17 +182,34 @@ class OMPLoopScope : public 
CodeGenFunction::RunCleanupsScope {
 }
 return false;
   });
-  PreInits = cast_or_null(LD->getPreInits());
+  PreInits = LD->getPreInits();
 } else if (const auto *Tile = dyn_cast(&S)) {
-  PreInits = cast_or_null(Tile->getPreInits());
+  PreInits = Tile->getPreInits();
 } else if (const auto *Unroll = dyn_cast(&S)) {
-  PreInits = cast_or_null(Unroll->getPreInits());
+  PreInits = Unroll->getPreInits();
 } else {
   llvm_unreachable("Unknown loop-based directive kind.");
 }
 if (PreInits) {
-  for (const auto *I : PreInits->decls())
-CGF.EmitVarDecl(cast(*I));
+  // CompoundStmts and DeclStmts are used as lists of PreInit statements 
and
+  // declarations. Since declarations must be visible in the the following
+  // that they initialize, unpack the ComboundStmt they are nested in.
+  SmallVector PreInitStmts;
+  if (auto *PreInitCompound = dyn_cast(PreInits))

Meinersbur wrote:

This is explained in the summary. In essence, the init-statement for a C++20 
foreach-loop does not need to be a DeclStmt, but can be an arbitrary Stmt.

https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [openmp] [Clang][OpenMP] Fix tile/unroll on iterator- and foreach-loops. (PR #91459)

[Michael Kruse via cfe-commits]

https://github.com/Meinersbur edited 
https://github.com/llvm/llvm-project/pull/91459
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits