[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops

[Srishti Srivastava via Phabricator via cfe-commits]

srishti-pm marked 4 inline comments as done.
srishti-pm added inline comments.


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:76
+      // `CONSTANT_OP` and `opName` remains "".
+      type = CONSTANT_OP;
+    }
----------------
Mogball wrote:
> Constant ops could be sorted by name as well.
The only reason we separated constant ops from the non-constant ops was because the former are canonicalized to the right (as a stable sort) by existing canonicalizations. And, we didn't want our algorithm to conflict with these existing canonicalizations. That is the reason I am not sorting them by name and just keeping them to the right (as a stable sort).


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:269
+    ArrayRef<std::unique_ptr<OperandBFS>> bfsOfOperands,
+    bool &hasOneOperandWithKey) {
+  bool keyFound = false;
----------------
Mogball wrote:
> This flag is not necessary because you can just check `bfsOfOperandsWithKey.size() == 1`
`.size()` is an O(N) operation and that is why I usually try to avoid it. Do you still agree we should use it here? I understand that N is an expectedly small value.


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:278-279
+    if (compareKeys(key, currentKey) == 0) {
+      bfsOfOperandsWithKey.push_back(
+          std::make_unique<OperandBFS>(*bfsOfOperand));
+      if (keyFound)
----------------
Mogball wrote:
> You don't need to make a copy. In fact, I think you should just track the indices.
I agree. But, we had discussed to store operands instead of indices and that's why I did this. I will change this to use indices again (keeping other things unchanged).


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:329-353
+  assert(frontPosition >= 0 && frontPosition < bfsOfOperands.size() &&
+         "`frontPosition` should be valid");
+  unsigned positionOfOperandToShift;
+  bool foundOperandToShift = false;
+  for (auto &indexedBfsOfOperand : llvm::enumerate(bfsOfOperands)) {
+    std::unique_ptr<OperandBFS> &bfsOfOperand = indexedBfsOfOperand.value();
+    if (bfsOfOperand->isSorted)
----------------
Mogball wrote:
> There is no way you need this much code. A `std::swap` between the current operand and the first unsorted position should be enough.
If I do a swap, the sorting will no longer be stable and I believe that there was a discussion that concluded with the fact that "we want stable sorting".


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:360
+/// the smallest position containing an unsorted operand).
+static void shiftTheSmallestUnsortedOperandsToTheSmallestUnsortedPositions(
+    SmallVectorImpl<std::unique_ptr<OperandBFS>> &bfsOfOperands,
----------------
Mogball wrote:
> This is possibly the longest function name I've ever seen. Please make it more concise.
Could you please give a suggestion for the name? After a long thought, I came up with this name. It was better than all my other ideas.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750