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

[Jeff Niu via Phabricator via cfe-commits]

Mogball added inline comments.


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:76
+      // `CONSTANT_OP` and `opName` remains "".
+      type = CONSTANT_OP;
+    }
----------------
srishti-pm wrote:
> 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).
I know. You can sort them separately from regular ops and also by name and the overall behaviour would be the same.


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:269
+    ArrayRef<std::unique_ptr<OperandBFS>> bfsOfOperands,
+    bool &hasOneOperandWithKey) {
+  bool keyFound = false;
----------------
srishti-pm wrote:
> 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.
Size is constant time 


================
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)
----------------
srishti-pm wrote:
> 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).
I mean you can have list (not a set) of indices to shift


================
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)
----------------
srishti-pm wrote:
> 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".
That's true, but shifting like this is very slow as well. At this point, you might want to give `std::stable_sort` with a custom comparator that does extra BFS iterations on demand a try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750