[PATCH] D130188: [DependenceAnalysis][PR56275] Normalize dependence analysis results to be non-negative when required

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DependenceAnalysis.h:254-255
 
+    /// Check if the direction vector is negative. A negative direction
+    /// vector means Src and Dst are reversed in the actual program.
+    bool isDirectionNegative() const override;
----------------
Meinersbur wrote:
> Please also mention the case that a dependency object can represent a lexicographically forward and reversed dependency at the same time, as discussed in https://github.com/llvm/llvm-project/issues/56275#issuecomment-1183363002 and what the expected behaviour for this case is.
Thanks, I've added the corresponding description in the comment of `isDirectionNegative()` in `DependenceAnalysis.cpp`.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:280
+  for (unsigned Level = 1; Level <= Levels; ++Level) {
+    auto Direction = DV[Level - 1].Direction;
+    if (Direction == Dependence::DVEntry::EQ)
----------------
Meinersbur wrote:
> [style] avoid Almost-Always-Auto
Thanks, updated accordingly.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:291
+
+bool FullDependence::normalize(ScalarEvolution *SE) {
+  if (!isDirectionNegative())
----------------
Meinersbur wrote:
> [serious] This should return a new `FullDependence` object. Modifying the existing object will also modify the object in the pass manager's analysis cache and potentially break any code using it and not expect it to be normalized.
Thanks, I've updated to make `normalize()` return a new FullDependence object (although copy constructor of `FullDependence` is not allowed), please refer to the most recent update. 


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:124-127
+static cl::opt<bool> NormalizeResults(
+    "da-normalize-results", cl::init(false), cl::Hidden,
+    cl::desc("Normalize the direction and distance vectors to make "
+             "them non-negative."));
----------------
Meinersbur wrote:
> congzhe wrote:
> > Meinersbur wrote:
> > > Meinersbur wrote:
> > > > [serious] This flag breaks any code that expects non-normalized dependences.
> > > An option for the `print-da` pass would be the better approach. Alternatively, if a dependency is normalized, also print the normalized dependence using a different output format. For instance:
> > > ```
> > > da analyze - flow [> <>]!
> > > da normalized - anti [< <>]!
> > > ```
> > Thanks a lot, considering both Bardia's and your comment, I've updated the patch such that the `DependenceInfo` constructor is not changed. Now the `NormalizeResults` option is only used in `print-da` pass. Also the output for the printer pass is also updated according to both of your comments.
> While `cl::opt` setting is acceptable, NPM explicitly have the possibility to parse pass options. Why not use it?
Thanks for this -- I'm not very clear of this functionality, I'm wondering if you could point me to some references?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:4179
+    Direction =
+        (Direction & 0x04) >> 2 | (Direction & 0x01) << 2 | (Direction & 0x02);
+    Result.DV[Level - 1].Direction =
----------------
Meinersbur wrote:
> congzhe wrote:
> > Meinersbur wrote:
> > > Can we avoid bit-operations with magic constants? 0x01 is `LT`, 0x02 is `EQ`, 0x04 is `GT`. 0x07 is `ALL`.
> > Thanks for the comment. I've added more detailed source code comments to describe the funtionality here, please refer to the updated patch.
> > 
> > I'd like to mention that those `0x01`, `0x02`, `0x04` are not directly related to `LT`/`EQ`/`GT`. They serve as the purpose of extracting the 1st, 2nd and 3rd bit (to the right), and then reversing the 1st and 3rd bits. This inherently reverses the direction vector, however complex it is, e.g., `banerjee6()` in the test file `banerjee.ll`.
> `LT`/`EQ`/`GT` //are // directly related to those bits. Swapping the `LT` <=> `GT` flags directly corresponds to swapping the 1st and 3rd bit. If it did not, the bit swapping would not work either and one would need to map all 2^6 possibilities separately. Note that `<>` is possible as well: `0x5`.
> 
> This is an alternative with less bit magic:
> ```
>     // reverse direction vector
>     // less flag becomes greater flag -- greater flag becomes less flag
>     unsigned char RevDirection = Direction & Dependence::DVEntry::EQ;
>     if (Direction & Dependence::DVEntry::LT)
>         RevDirection |= Dependence::DVEntry::GT;
>     if (Direction & Dependence::DVEntry::GT)
>         RevDirection |= Dependence::DVEntry::LT;
> ```
Thanks a lot for the suggestion, I've updated the code as per your suggestion.


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

https://reviews.llvm.org/D130188