[PATCH] D130189: [LoopInterchange][PR56275] Fix legality in dependence vectors

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:131
+        if (D->isDirectionNegative())
+          D->normalize(SE);
         LLVM_DEBUG(StringRef DepType =
----------------
Before I can accept the patch, I would like to see D130188 to treat a `Dependence` object as immutable (modifying global state is incompatible with pass manager analysis caching)


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:145-146
               Direction = '=';
             else
-              Direction = '>';
+              Direction = '<';
             Dep.push_back(Direction);
----------------
congzhe wrote:
> Meinersbur wrote:
> > [serious] A SCEV can be neither `isPositive()`, `isZero()`, nor `isNegative()`. This is for instance that case when it depend on a variable which can be either one of them at runtime. This code implicitly assumes that such a distance is "negative". It should be '*'.
> Thanks for the comment. Thinking about this piece of code more carefully, I think the entire `if (SCEVConst)` block can be deleted. This code block is merely a subset of the the `else` block where we could just leverage `Dir = D->getDirection(II)` to construct `Dep`. Another benefit is that the `else` block explicitly addresses your concern - if the direction is bidirectional, it would be a "*".
I think you are correct. If the distance is positive/negative, the direction be `<`/`>` anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130189/new/

https://reviews.llvm.org/D130189