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

[Ramkrishnan Narayanan Komala via Phabricator via llvm-commits]

ram-NK 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,
----------------
bmahjour wrote:
> I don't see why we need two options to control the legacy cost model.
This new parameter added as per @Meinersbur comments in [[ https://reviews.llvm.org/D135808?id=474010#inline-1332682 | Inline]]. This is considered as the upper limit of InstrOrderCost.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1138
+      return std::optional<bool>(true);
+    else
+      return std::optional<bool>(false);
----------------
bmahjour wrote:
> We should return `nullopt` when/if `InnerIndex == OuterIndex`
The CostMap is assigned with unique number to each loop. So this condition "InnerIndex == OuterIndex" will not satisfy. But LoopCost may be same, CC->getLoopCost(*InnerLoop) == CC->getLoopCost(*OuterLoop) checking can be added for returning  nullopt.


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

https://reviews.llvm.org/D135808