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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:64
 
+static cl::opt<int> LegacyCostModelThreshold(
+    "legacy-cost-threshold", cl::init(10), cl::Hidden,
----------------
I don't see why we need two options to control the legacy cost model.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1138
+      return std::optional<bool>(true);
+    else
+      return std::optional<bool>(false);
----------------
We should return `nullopt` when/if `InnerIndex == OuterIndex`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1164
+  // issues).
+  if (CC && CC->getLoopCost(*InnerLoop) == CC->getLoopCost(*OuterLoop)) {
+    for (auto &Row : DepMatrix) {
----------------
since the idea is to call this function only when CC has failed or been indecisive, we don't need to pass CC to this function and check it here.


================
Comment at: llvm/test/Transforms/LoopInterchange/profitability.ll:171-176
+;; Loop interchange needed for better locality as per Cache analysis.
+;; Again loop-interchange is profitable as per isProfitableForVectorization.
+;; There would be endless interchange (no proper priority was not there in profitability check.
+;; Any profitable check may lead to loop-interchange) before D135808.
+;; And there is no endless interchange  after D135808 (priority in profitability check is defined.
+;; Order of decision is Cache cost check, InstrOrderCost , Vectorization ).
----------------
reword:

This tests to make sure, that multiple invocations of loop interchange will not undo previous interchange and will converge to a particular order determined by the profitability analysis. 


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

https://reviews.llvm.org/D135808