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

[Srishti Srivastava via Phabricator via cfe-commits]

srishti-pm added a comment.

@mehdi_amini, I have made sure that the algorithm is good in terms of both time and space complexity.

@Mogball, "handling attributes (e.g. cmpi slt vs cmpi sgt)" doesn't seem hard to me. I think this algorithm can be extended with ease to take attributes into account. But, I don't think that it should be a part of this revision (I believe you agree) because it seems like an incremental change which should be added only when the need arises. A new user should first get accustomed to this utility (and how its sorts operands with different backward slices) and then the utility can be extended to differentiate between backward slices containing ops with the same name but different attribute values.



================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:34
+/// Stores the "key" associated with a block argument or an operation.
+struct BlockArgumentOrOpKey {
+  /// Holds `BLOCK_ARGUMENT`, `NON_CONSTANT_OP`, or `CONSTANT_OP`, depending on
----------------
Mogball wrote:
> `using BlockArgumentOrOpKey = std::pair<BlockArgumentOrOpType, StringRef>`
> 
> The default `operator<` for `std::pair` should work for you.
I have added a constructor to `BlockArgumentOrOpKey` (now renamed to `AncestorKey`) and thus I think this comment is obsolete now. Hope that this fine. Adding a constructor made the code look cleaner.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:226
+    for (Value operand : op->getOperands()) {
+      OperandBFS *bfsOfOperand = new OperandBFS();
+      bfsOfOperand->pushAncestor(operand.getDefiningOp());
----------------
Mogball wrote:
> memory leak?
Fixed this by adding a struct called "Ancestor" which refers to either an op or a block argument.


================
Comment at: mlir/include/mlir/Transforms/CommutativityUtils.h:257
+      // smallest.
+      DenseSet<unsigned> smallestKeyIndices;
+      // Stores the indices of the unassigned operands whose key is the largest.
----------------
Mogball wrote:
> Could you not change `getIndicesOfUnassigned...` to populate two lists of operands and pass these to `assignSortedPositionTo` instead of using a set to track the indices. You could put the operand index inside `OperandBFS` to keep track.
I think that doing this might not be a good idea. It will increase the space complexity unnecessarily (OperandBFS is a big structure) and not help much with the time complexity because the sets of the indices are expected to be small. At least the number of indices will be <= the total number of operands and each element in these sets will occupy very less space (size of `unsigned`).


================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:271
+  for (Operation *operandDefOp : operandDefOps) {
+    OperandBFS *bfsOfOperand = new OperandBFS();
+    bfsOfOperand->pushAncestor(operandDefOp);
----------------
mehdi_amini wrote:
> Is this a leak?
> 
Fixed this by adding a struct called "Ancestor" which refers to either an op or a block argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124750