[clang] [Clang][OpenMP][LoopTransformations] Fix incorrect number of generated loops for Tile and Reverse directives (PR #140532)
Walter J.T.V via cfe-commits
cfe-commits at lists.llvm.org
Mon Jun 16 00:42:14 PDT 2025
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
More information about the cfe-commits
mailing list