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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
