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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DependenceAnalysis.h:960
+    /// directions and distances in the vector.
+    void normalize(FullDependence &Result);
   }; // class DependenceInfo
----------------
congzhe wrote:
> 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.
Sure, but normalize() could still return a boolean indicating if it changed the Dependence object. 


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:125
+static cl::opt<bool> NormalizeResults(
+    "da-normalize-results", cl::init(false), cl::Hidden,
+    cl::desc("Normalize the direction and distance vectors to make "
----------------
Suggestion:
`NormalizeResults` -> `NormalizePrintedResults`
`da-normalize-results` -> `da-print-normalized-results`


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:198
+              D->normalize(&SE);
+              OS << "**normalized** ";
+            }
----------------
Suggestion for better readability: "** normalized **"


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:297
+
+  std::swap(Src, Dst);
+  for (unsigned Level = 1; Level <= Levels; ++Level) {
----------------
This function should call `isDirectionNegative()` to determine if reversing of the direction vector is necessary. If the direction is not negative, then it should not make any changes and return false. Otherwise proceed and return true.


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

https://reviews.llvm.org/D130188