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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour accepted this revision.
bmahjour added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.

LGTM with some minor comments.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1150
+    // parallelism.
+    if (Row[OuterLoopId] != '=')
+      return std::optional<bool>(false);
----------------
ideally this should be

`   if (Row[OuterLoopId] != '=' && Row[OuterLoopId] != 'I')`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1138
+      return std::optional<bool>(true);
+    else
+      return std::optional<bool>(false);
----------------
ram-NK wrote:
> 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.
ah, ok...in that case please add `assert(InnerIndex != OuterIndex && "CostMap should assign unique numbers to each loop")`


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

https://reviews.llvm.org/D135808