[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: Thank you so much @Meinersbur ! https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
Meinersbur wrote: > Well yes but i don't have enough permissions to merge. I didn't know. Additional changes are find, I just merged it. Thanks for the patch. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
github-actions[bot] wrote: @eZWALT Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our [build bots](https://lab.llvm.org/buildbot/). If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail [here](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr). If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of [LLVM development](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/Meinersbur closed https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
Meinersbur wrote:
> Thank you! But before accepting the merge, does this mean that we discard
> everything that i said about refactoring these variables and just keep it as
> it is in both reviews @alexey-bataev ? Also, could you @Meinersbur specify
> what exactly do you mean with a regression test for such thing? You mean like
> incorporating some kind of logic which includes NumGeneratedLoops in all
> LoopTransformation directives or just tile and reverse? Just want to be sure
> before proceeding with #139293, in the aforementioned case it would not need
> further revision.
Whatever the refactoring will be, this I think this patch is useful by itself.
Even if the refactoring will delete all this code, the commit can be
cherry-picked for those who do not want the refactoring. I don't think we
should work forever on obvious fixes in case we can find something nicer.
With the reproducer I meant the `setNumGeneratedLoops(1);` for the reverse
directive. It might make a difference in
`OMPLoopBasedDirective::doForAllLoop()` which might think there are no further
loops to iterate into (`if (NumGeneratedLoops == 0) {`) when in fact there is.
This requires multiple nesting of loop transformations which might be difficult
to find a reproducer for. Since I think this fix is rather obvious, I would
prioritize fixing it over spending a lot of time finding a regression test.
https://github.com/llvm/llvm-project/pull/140532
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
rofirrim wrote: > I'm guessing that is a yes, could you @alexey-bataev please approve the > workflows and we proceed to merge? Then we can go straight into the > https://github.com/llvm/llvm-project/pull/139293 PR :) I did that already. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/eZWALT updated
https://github.com/llvm/llvm-project/pull/140532
>From affda91204c1aacdab8ebd0966a27e93feec6db3 Mon Sep 17 00:00:00 2001
From: eZWALT
Date: Mon, 19 May 2025 10:49:10 +
Subject: [PATCH] Correct the number of generated loops
---
clang/include/clang/AST/StmtOpenMP.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
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) {
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > > I originally kept the `NumGeneratedLoops` information consistent despite > > being partially subsumed by NumGeneratedLoopNests (Note that its not > > actually the same, it returns the number of generated loops in total adding > > nested loops, but due to the current usage of this semantic information I > > could argue they serve the same purpose) just in case this information was > > going to be used for future OpenMP Transformations or semantic logic maybe > > in OpenMP 7.0. If you both think this information about nested loops will > > never be used (Maybe if fusion gets a clause for multi-level fusion / > > fission could become relevant...), then I just remove it, but instead of > > storing a boolean value, a boolean function `hasGeneratedLoops` = > > `self->NumGeneratedLoops > 0` could be coded. > > After revising this topic thoroughly, I believe the most reasonable course > > of action would be to close this PR and keep `NumGeneratedLoops` as it is > > currently in this patch. Then, swap the semantic information of > > NumGeneratedLoops <-> `NumGeneratedLoopNests` and remove > > NumGeneratedLoopNests in patch #139293 . This would also enable me to > > remove unnecessary computations that were performed in > > `AnalyzeLoopSequence` to keep `NumGeneratedLoops` consistent, simplifying > > logic and removing this second variable. > > @alexey-bataev , @Meinersbur what do you think? Will this information ever > > be needed? > > Thank you! But before accepting the merge, does this mean that we discard > everything that i said about refactoring these variables and just keep it as > it is in both reviews @alexey-bataev ? Also, could you @Meinersbur specify > what exactly do you mean with a regression test for such thing? You mean like > incorporating some kind of logic which includes NumGeneratedLoops in all > LoopTransformation directives or just tile and reverse? Just want to be sure > before proceeding with #139293, in the aforementioned case it would not need > further revision. I will assume no answer means yes, thank you! https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > I originally kept the `NumGeneratedLoops` information consistent despite > being partially subsumed by NumGeneratedLoopNests (Note that its not actually > the same, it returns the number of generated loops in total adding nested > loops, but due to the current usage of this semantic information I could > argue they serve the same purpose) just in case this information was going to > be used for future OpenMP Transformations or semantic logic maybe in OpenMP > 7.0. If you both think this information about nested loops will never be used > (Maybe if fusion gets a clause for multi-level fusion / fission could become > relevant...), then I just remove it, but instead of storing a boolean value, > a boolean function `hasGeneratedLoops` = `self->NumGeneratedLoops > 0` could > be coded. > > After revising this topic thoroughly, I believe the most reasonable course of > action would be to close this PR and keep `NumGeneratedLoops` as it is > currently in this patch. Then, swap the semantic information of > NumGeneratedLoops <-> `NumGeneratedLoopNests` and remove > NumGeneratedLoopNests in patch #139293 . This would also enable me to remove > unnecessary computations that were performed in `AnalyzeLoopSequence` to keep > `NumGeneratedLoops` consistent, simplifying logic and removing this second > variable. > > @alexey-bataev , @Meinersbur what do you think? Will this information ever be > needed? Thank you! But before accepting the merge, does this mean that we discard everything that i said about refactoring these variables and just keep it as it is in both reviews? Also, could you @Meinersbur specify what exactly do you mean with a regression test for such thing? You mean like incorporating some kind of logic which includes NumGeneratedLoops in all LoopTransformation directives or just tile and reverse? https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/alexey-bataev approved this pull request. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/Meinersbur approved this pull request. I am OK with this patch as-is, independented of #139293 since it was cleary a (my) mistake. Just a test case to detect regressions would be nice if possible. So I LGTM this patch. If @alexey-bataev wants additional changes, this can be done independently. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: gentle ping @alexey-bataev @Meinersbur ;) https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: I originally kept the `NumGeneratedLoops` information consistent despite being partially subsumed by NumGeneratedLoopNests (Note that its not actually the same, it returns the number of generated loops in total adding nested loops, but due to the current usage of this semantic information I could argue they serve the same purpose) just in case this information was going to be used for future OpenMP Transformations or semantic logic maybe in OpenMP 7.0. If you both think this information about nested loops will never be used (Maybe if fusion gets a clause for multi-level fusion / fission could become relevant...), then I just remove it, but instead of storing a boolean value, a boolean function `hasGeneratedLoops` = `self->NumGeneratedLoops > 0` could be coded. After revising this topic thoroughly, I believe the most reasonable course of action would be to close this PR and keep `NumGeneratedLoops` as it is currently in this patch. Then, swap the semantic information of NumGeneratedLoops <-> `NumGeneratedLoopNests` and remove NumGeneratedLoopNests. This would also enable me to remove unnecessary computations that were performed in `AnalyzeLoopSequence` to keep `NumGeneratedLoops` consistent, simplifying logic and removing this second variable. @alexey-bataev , @Meinersbur what do you think? Will this information ever be needed? https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
Meinersbur wrote: This change makes sense to me. In principle, the information of generated loops is only needed in dependent contexts (not yet fully expanded templates); elsewhere we get the loop nest structure from `getTransformedStmt()`. The use in `doForAllLoops` is meant to delay evaluation for when the template is instantiated. A QoI would be able to check the loop structure within a template from this information, even without it needed to be instantiated. I don't think we should remove the information only to be re-added again for #139293. I think a test case could be possible under the following conditions: 1. reverse construct (number of generated loops of `tile` is positive in either case) 2. In a template definition (dependent context) 3. Loop bounds of reversed loop depends on template parameter (seeing https://github.com/llvm/llvm-project/blob/dedef408d759b50360ae8a7ef2ba13ba6931b4d8/clang/lib/Sema/SemaOpenMP.cpp#L15136 this might not be necessary) 4. Another construct applied to the generated loop of the `reverse` (e.g. unroll) In that case `doForAllLoops` should `break` instead of `return` causing the following code to execute on the `OMPReverseConstruct` itself instead of a loop, causing an assertion to fail. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: gentle ping :) https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > > AnalyzeLoopSequence > > Could you point, where this is located? Although now that i think about it, this function is not inside this PR but rather on the PR #139293, but the dependency is clear. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > > AnalyzeLoopSequence > > Could you point, where this is located? Of course, [See line 14315 in `SemaOpenMP.cpp`](https://github.com/eZWALT/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L14315) https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: > AnalyzeLoopSequence Could you point, where this is located? https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: gentle ping @alexey-bataev https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > > > 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. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: > > 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 https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > 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. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: 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 https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > > @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](https://github.com/llvm/llvm-project/pull/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`. **Summing up**: preserving this semantic information is important, and there’s no reason to change it right now. Feel free to digress, but I’m confident this is the right approach. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: > @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](https://github.com/llvm/llvm-project/pull/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 https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/eZWALT updated
https://github.com/llvm/llvm-project/pull/140532
>From affda91204c1aacdab8ebd0966a27e93feec6db3 Mon Sep 17 00:00:00 2001
From: eZWALT
Date: Mon, 19 May 2025 10:49:10 +
Subject: [PATCH] Correct the number of generated loops
---
clang/include/clang/AST/StmtOpenMP.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
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) {
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: 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: #139293), specifically within `checkTransformableLoopSequence`, where both `NumGeneratedLoops` and `NumGeneratedLoopNests` are read and actively utilized. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > 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. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: It would be good to try to find the cases that may reveal this issues before committing the patch https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: Before leaving i can attest that the regression tests have been passed twice :+1: https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > 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 https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: Are there any tests that might be affected by this change? https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: > @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. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
eZWALT wrote: 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. https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
alexey-bataev wrote: Do we need the number of generated loops at all? Is it used anywhere? Maybe it worth it to remove it? https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/eZWALT edited https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
github-actions[bot] wrote: 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](https://llvm.org/docs/GitHub.html). You can also ask questions in a comment on this PR, on the [LLVM Discord](https://discord.com/invite/xS7Z362) or on the [forums](https://discourse.llvm.org/). https://github.com/llvm/llvm-project/pull/140532 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
llvmbot wrote:
@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
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) {
``
https://github.com/llvm/llvm-project/pull/140532
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
https://github.com/eZWALT created
https://github.com/llvm/llvm-project/pull/140532
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.
>From affda91204c1aacdab8ebd0966a27e93feec6db3 Mon Sep 17 00:00:00 2001
From: eZWALT
Date: Mon, 19 May 2025 10:49:10 +
Subject: [PATCH] Correct the number of generated loops
---
clang/include/clang/AST/StmtOpenMP.h | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
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) {
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
