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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
