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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur 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;
----------------
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.


================
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)
----------------
[style] avoid Almost-Always-Auto


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:291
+
+bool FullDependence::normalize(ScalarEvolution *SE) {
+  if (!isDirectionNegative())
----------------
[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.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:302
+    // those bits. This essentially reverses the direction.
+    Direction = (Direction & 0x07);
+    // Based on the bit representation of "direction", it is sufficient to reverse
----------------
`.Direction` is a bitfield. The compiler already does the masking for you.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:430
 
-
 // Updates X with the intersection
----------------
[nit] unrelated change


================
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."));
----------------
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?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:4179
+    Direction =
+        (Direction & 0x04) >> 2 | (Direction & 0x01) << 2 | (Direction & 0x02);
+    Result.DV[Level - 1].Direction =
----------------
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;
```


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

https://reviews.llvm.org/D130188