[PATCH] D135808: [LoopInterchange] Correcting the profitability check

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1124
 
-static bool isProfitableForVectorization(unsigned InnerLoopId,
-                                         unsigned OuterLoopId,
-                                         CharMatrix &DepMatrix) {
-  // TODO: Improve this heuristic to catch more cases.
-  // If the inner loop is loop independent or doesn't carry any dependency it is
-  // profitable to move this to outer position.
-  for (auto &Row : DepMatrix) {
-    if (Row[InnerLoopId] != 'S' && Row[InnerLoopId] != 'I')
-      return false;
-    // TODO: We need to improve this heuristic.
-    if (Row[OuterLoopId] != '=')
-      return false;
-  }
-  // If outer loop has dependence and inner loop is loop independent then it is
-  // profitable to interchange to enable parallelism.
-  // If there are no dependences, interchanging will not improve anything.
-  return !DepMatrix.empty();
-}
-
-bool LoopInterchangeProfitability::isProfitable(
-    const Loop *InnerLoop, const Loop *OuterLoop, unsigned InnerLoopId,
-    unsigned OuterLoopId, CharMatrix &DepMatrix,
-    const DenseMap<const Loop *, unsigned> &CostMap) {
-  // TODO: Remove the legacy cost model.
-
+std::optional<bool> LoopInterchangeProfitability::isProfitableAccordingLoopCacheAnalysis(
+       const DenseMap<const Loop *, unsigned> &CostMap) {
----------------
nit: `isProfitableAccordingToLoopCacheAnalysis`, or `isProfitablePerLoopCacheAnalysis`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1144
 
-  // It is not profitable as per current cache profitability model. But check if
-  // we can move this loop outside to improve parallelism.
-  if (isProfitableForVectorization(InnerLoopId, OuterLoopId, DepMatrix))
-    return true;
+std::optional<bool> LoopInterchangeProfitability::isProfitableAccordingInstrOrderCost() {
+  // Legacy cost model: this is rough cost estimation algorithm. It counts the
----------------
nit: `isProfitableAccordingToInstrOrderCost`, or `isProfitablePerInstrOrderCost`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1185
+
+bool LoopInterchangeProfitability::isProfitable(
+    const Loop *InnerLoop, const Loop *OuterLoop, unsigned InnerLoopId,
----------------
It could be worth adding some comments for this function that describe what it does now, and how it prevents endless interchange from happening.


================
Comment at: llvm/test/Transforms/LoopInterchange/profitability.ll:175
+;; Any profitable check may leads to loop-interchange) before.
+;; And there is no endless interchange now (priority in profitability check is defined.
+;; Order of decision is Cache cost check, InstrOrderCost , Vectorization ).
----------------
nit: 

`may leads`->`may lead`
`before`-> `before D135808`
`now`-> `after D135808`


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

https://reviews.llvm.org/D135808