[Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives

[eZWALT]

This patch is closely related to #139293 and addresses an existing issue in the loop transformation codebase. Specifically, it corrects the handling of the NumGeneratedLoops variable in OMPLoopTransformationDirective AST nodes and its inheritors (such as OMPUnrollDirective, OMPTileDirective, etc.).

Previously, this variable was inaccurately set for certain transformations like reverse or tile. While this did not lead to functional bugs, since the value was only checked to determine whether it was greater than zero or equal to zero, the inconsistency could introduce problems when supporting more complex directives in the future.

@alexey-bataev , @Meinersbur

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Walter J.T.V (eZWALT)

Changes

This patch is closely related to #139293 and addresses an existing issue in the loop transformation codebase. Specifically, it corrects the handling of the NumGeneratedLoops variable in OMPLoopTransformationDirective AST nodes and its inheritors (such as OMPUnrollDirective, OMPTileDirective, etc.).

Previously, this variable was inaccurately set for certain transformations like reverse or tile. While this did not lead to functional bugs, since the value was only checked to determine whether it was greater than zero or equal to zero, the inconsistency could introduce problems when supporting more complex directives in the future.

---

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

1 Files Affected:

• (modified) clang/include/clang/AST/StmtOpenMP.h (+4-2)

diff --git a/clang/include/clang/AST/StmtOpenMP.h b/clang/include/clang/AST/StmtOpenMP.h
index 736bcabbad1f7..7ded194dd6eb2 100644
--- a/clang/include/clang/AST/StmtOpenMP.h
+++ b/clang/include/clang/AST/StmtOpenMP.h
@@ -5790,7 +5790,9 @@ class OMPReverseDirective final : public OMPLoopTransformationDirective {
   explicit OMPReverseDirective(SourceLocation StartLoc, SourceLocation EndLoc)
       : OMPLoopTransformationDirective(OMPReverseDirectiveClass,
                                        llvm::omp::OMPD_reverse, StartLoc,
-                                       EndLoc, 1) {}
+                                       EndLoc, 1) {
+    setNumGeneratedLoops(1);
+  }
 
   void setPreInits(Stmt *PreInits) {
     Data->getChildren()[PreInitsOffset] = PreInits;
@@ -5857,7 +5859,7 @@ class OMPInterchangeDirective final : public OMPLoopTransformationDirective {
       : OMPLoopTransformationDirective(OMPInterchangeDirectiveClass,
                                        llvm::omp::OMPD_interchange, StartLoc,
                                        EndLoc, NumLoops) {
-    setNumGeneratedLoops(3 * NumLoops);
+    setNumGeneratedLoops(NumLoops);
   }
 
   void setPreInits(Stmt *PreInits) {

[alexey-bataev]

Do we need the number of generated loops at all? Is it used anywhere? Maybe it worth it to remove it?

[eZWALT]

@alexey-bataev
It’s true that NumGeneratedLoops is used throughout the existing OpenMP loop transformation infrastructure. While in some cases its usage could potentially be replaced by NumGeneratedLoopNests (especially when only checking for values like 0 or 1), the two variables convey distinct semantic information.

NumGeneratedLoops refers to the number of individual loops produced, while NumGeneratedLoopNests captures the structure of nested loops. For current and future transformations, having access to both could be important for representing complex constructs accurately.

Removing NumGeneratedLoops would require changes across the loop transformations logic it's not clear the benefit would justify that cost, particularly given the potential utility of retaining this semantic distinction.I’m not 100% certain all current transformations depend on that level of detail, but I believe it’s valuable to preserve until proven otherwise.

[eZWALT]

@alexey-bataev It’s true that NumGeneratedLoops is used throughout the existing OpenMP loop transformation infrastructure. While in some cases its usage could potentially be replaced by NumGeneratedLoopNests (especially when only checking for values like 0 or 1), the two variables convey distinct semantic information.

NumGeneratedLoops refers to the number of individual loops produced, while NumGeneratedLoopNests captures the structure of nested loops. For current and future transformations, having access to both could be important for representing complex constructs accurately.

Removing NumGeneratedLoops would require changes across the loop transformations logic it's not clear the benefit would justify that cost, particularly given the potential utility of retaining this semantic distinction.I’m not 100% certain all current transformations depend on that level of detail, but I believe it’s valuable to preserve until proven otherwise.

I've identified a case where NumGeneratedLoops is necessary and cannot be replaced by NumGeneratedLoopNests: the permutation clause of the interchange directive, e.g., permutation(2,1,...). In this transformation, we’re not interested in the number of top-level loop nests, but rather in how many individual loops exist within a single top-level nest, and how to reorder them. Let me know if i have clarified your doubts or if you want more examples, sometimes this kind of details are somewhat difficult to explain easily.

[alexey-bataev]

Are there any tests that might be affected by this change?

[eZWALT]

Are there any tests that might be affected by this change?

Yesterday I ran all the tests (check-clang-openmp and check-clang) and no change in the behaviour or incidence was found, although i'll re-execute them just as a sanity check. Just a remainder but this merge request should be merged firstly before #139293.

[alexey-bataev]

It would be good to try to find the cases that may reveal this issues before committing the patch

[eZWALT]

It would be good to try to find the cases that may reveal this issues before committing the patch

Yes, i'll go through loop transformations tests and notify you tomorrow if this is the case, but i'm pretty sure that these are not breaking changes for the same reason that i told you on the other PR, NumGeneratedLoops is almost only being used in the CheckTransformableLoopNest and well i will have to check both ActOnOMPReverse/InterchangeDirective

[eZWALT]

Before leaving i can attest that the regression tests have been passed twice 👍

[eZWALT]

@alexey-bataev
After conducting an examination of the directive handling logic, I can confidently state that the number of generated loops (NumGeneratedLoops) does not affect the semantic checks for the majority of transformations. This is because values are usually hardcoded in the ActOnXXX semantic handlers. For example:

• In the case of the 'reverse' directive, the number of loops (NumLoops) is hardcoded to 1, meaning it remains unaffected by any external loop count logic.

• For the 'interchange' directive, the number of loops is also explicitly set using the following logic:

size_t NumLoops = PermutationClause ? PermutationClause->getNumLoops() : 2;

These values are passed into the checkTransformableLoopNest function and are not accessed elsewhere in the codebase, except:

• In doForAllLoops, where only the presence of loops is checked (i.e., NumLoops == 0 or > 0), not the actual count, therefore this change won't break this conditional flow.
• In the newly introduced analysis functions (as part of the 'fuse' transformation: [Clang][OpenMP][LoopTransformations] Add support for "#pragma omp fuse" loop transformation directive and "looprange" clause #139293), specifically within checkTransformableLoopSequence, where both NumGeneratedLoops and NumGeneratedLoopNests are read and actively utilized.

[alexey-bataev]

@alexey-bataev After conducting an examination of the directive handling logic, I can confidently state that the number of generated loops (NumGeneratedLoops) does not affect the semantic checks for the majority of transformations. This is because values are usually hardcoded in the ActOnXXX semantic handlers. For example:

• In the case of the 'reverse' directive, the number of loops (NumLoops) is hardcoded to 1, meaning it remains unaffected by any external loop count logic.
• For the 'interchange' directive, the number of loops is also explicitly set using the following logic:

size_t NumLoops = PermutationClause ? PermutationClause->getNumLoops() : 2;

These values are passed into the checkTransformableLoopNest function and are not accessed elsewhere in the codebase, except:

• In doForAllLoops, where only the presence of loops is checked (i.e., NumLoops == 0 or > 0), not the actual count, therefore this change won't break this conditional flow.
• In the newly introduced analysis functions (as part of the 'fuse' transformation: [Clang][OpenMP][LoopTransformations] Add support for "#pragma omp fuse" loop transformation directive and "looprange" clause #139293), specifically within checkTransformableLoopSequence, where both NumGeneratedLoops and NumGeneratedLoopNests are read and actively utilized.

Can you remove these hardcoded values and use the stored value instead? Otherwise, it is meaningless and should be removed

[eZWALT]

@alexey-bataev After conducting an examination of the directive handling logic, I can confidently state that the number of generated loops (NumGeneratedLoops) does not affect the semantic checks for the majority of transformations. This is because values are usually hardcoded in the ActOnXXX semantic handlers. For example:

• In the case of the 'reverse' directive, the number of loops (NumLoops) is hardcoded to 1, meaning it remains unaffected by any external loop count logic.
• For the 'interchange' directive, the number of loops is also explicitly set using the following logic:

size_t NumLoops = PermutationClause ? PermutationClause->getNumLoops() : 2;

These values are passed into the checkTransformableLoopNest function and are not accessed elsewhere in the codebase, except:

• In doForAllLoops, where only the presence of loops is checked (i.e., NumLoops == 0 or > 0), not the actual count, therefore this change won't break this conditional flow.
• In the newly introduced analysis functions (as part of the 'fuse' transformation: [Clang][OpenMP][LoopTransformations] Add support for "#pragma omp fuse" loop transformation directive and "looprange" clause #139293), specifically within checkTransformableLoopSequence, where both NumGeneratedLoops and NumGeneratedLoopNests are read and actively utilized.

Can you remove these hardcoded values and use the stored value instead? Otherwise, it is meaningless and should be removed

But this rigidity stems from the checkTransformableLoopNest, which needs the number of loops to be specified beforehand. Changing this wouldn't make much sense. The NumGeneratedLoops information is only available after the creation of the OMPLoopTransformation AST nodes, but the number of loops must be known before that.

Note that inferring this knowledge is trivial in the old scheme of loop transformations, since almost all have one top-level loop, or the loop count is specified by a clause , for example, PermutationClause. OMPFuseDirective is the only one where the number of loops (both top-level and nested) is dynamic, depending on the user code. Therefore, it is mandatory to do an analysis to gather the shape of the loop sequence. Probably I haven’t explained myself well enough, but I want to stress the difference:

• NumLoops refers to the loops expected or known beforehand — which, in most directives, can be hardcoded.
• NumGeneratedLoops is the total number of loops after transformation, stored as semantic information in the AST.
• NumGeneratedLoopNests is the number of top-level loop nests, which is important for loop sequence transformations like fuse or split.

Preserving this semantic information is important, and there’s no reason to change it right now.

[alexey-bataev]

What I see in the source code that it is used as a boolean flag. Can we transform it to bool? There is no need to keep it integer

[eZWALT]

What I see in the source code that it is used as a boolean flag. Can we transform it to bool? There is no need to keep it integer

Please could you cite the exact line? I'm not sure if you are refering to the logic inside checkTransformableLoopNest or not.

[alexey-bataev]

What I see in the source code that it is used as a boolean flag. Can we transform it to bool? There is no need to keep it integer

Please could you cite the exact line? I'm not sure if you are refering to the logic inside checkTransformableLoopNest or not.

I check getNumGeneratedLoops() function usage. I see that it is used only in 2 linesб which can be replaced by just a boolean

[eZWALT]

What I see in the source code that it is used as a boolean flag. Can we transform it to bool? There is no need to keep it integer

Please could you cite the exact line? I'm not sure if you are refering to the logic inside checkTransformableLoopNest or not.

I check getNumGeneratedLoops() function usage. I see that it is used only in 2 linesб which can be replaced by just a boolean

We could add a boolean function like 'AreThereGeneratedLoops()', but getGeneratedNumLoops() is also used to count the total loops inside 'AnalyzeLoopSequence', which feeds into NumGeneratedLoops in OMPFuseDirective. Changing its return type would break that. While we could remove NumGeneratedLoops out of OMPLoopTransformation AST nodes, it provides useful semantic flexibility for future transformations. There’s a tradeoff, but I believe keeping it does more good than harm.