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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D130188#3669842 <https://reviews.llvm.org/D130188#3669842>, @bmahjour wrote:

> Since the `normalize()` function is conceptually an operation that applies to the result (as opposed to the analysis itself), it makes more sense for it to be a member of the `Dependence` class. Client code, such as loop interchange, would then do something like this:
>
>   if (auto D = DI->depends(Src, Dst, true).normalize()) {
>      ...
>
> This way the analysis can be done once, but the results can be interpreted differently if the need arises.

Thanks for the comment, I've updated the patch accordingly, i.e., make `normalize()` a member function of the `Dependence/FullDependence` class and hence it can be called as you suggested. Please refer to the loop interchange patch D130189 <https://reviews.llvm.org/D130189> as well.



================
Comment at: llvm/include/llvm/Analysis/DependenceAnalysis.h:960
+    /// directions and distances in the vector.
+    void normalize(FullDependence &Result);
   }; // class DependenceInfo
----------------
bmahjour wrote:
> It would be good for the `normalize()` function to return a boolean indicating if it changed the result or not. This could come in handy, for example, when printing the results (please see my comment about the printer pass).
Thanks for the comment, I understand your point. In fact  the "normalization" process is implemented in two-parts - `isDirectionNegative()` and `normalize()`. What `isDirectionNegative()` returns is already a `boolean` (which is the funcationality you requested), and the client can know whether or not the depenedence vector is negative, and can further decide whether or not it wants to call `normalize()`. 

Please also refer to the most recent change in `dumpExampleDependence()` for the uses of `isDirectionNegative()` and `normalize()`. I thought about merging those two function into one, but I thought keep them as two segement functions can make it easier if clients merely want to check if the direction is negative but do not want to normalize the result.


================
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:
> 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.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:4157-4159
+    if (Result.getDirection(Level) == Dependence::DVEntry::LT ||
+        Result.getDirection(Level) == Dependence::DVEntry::LE)
+      return false;
----------------
Meinersbur wrote:
> The code falls into the final `return false` anyway, i.e. this condition is redundant.
> 
> You could also use a `switch` statement.
Thanks, I've removed this redundant condition.


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


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:4181
+    Result.DV[Level - 1].Direction =
+        (Result.DV[Level - 1].Direction & 0xF8) | Direction;
+    // Reverse the dependence distance as well.
----------------
Meinersbur wrote:
> `.Direction` is a bitfield. The compiler already does the masking for you.
Thanks, updated accordingly.


================
Comment at: llvm/test/Analysis/DependenceAnalysis/Banerjee.ll:248
+; NORMALIZE: da analyze - none!
+; NORMALIZE: da analyze - anti [< <]!
+; NORMALIZE: da analyze - confused!
----------------
bmahjour wrote:
> Since the printer pass (using `dumpExampleDependence`) prints the Src and Dst instructions in a fixed order, these results could easily be misinterpreted. To avoid confusion can we flag the results that have been normalized with a special marker? eg:
> 
> ```
> Src: store....
> Dst: ...load
>   da analyze - anti [< <]! **normalized**
> ```
> 
Thanks, I've done the change accordingly. Please also refer to the most recent `NORMALIZE` lines in tests.


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

https://reviews.llvm.org/D130188