[PATCH] D124750: [MLIR] Add a utility to sort the operands of commutative ops
Mehdi AMINI via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon May 9 07:30:57 PDT 2022
mehdi_amini added a comment.
Seems like this should be added to canonicalization? The "push constants to the right hand side" is there already.
I also don't understand the complexity of the implementation, I may need an example to understand why you're recursively operating on the producer ops for the operands.
>From the revision description: `, (1) the operands defined by non-constant-like ops come first, followed by (2) block arguments, and these are followed by (3) the operands defined by constant-like ops` which all seems like a fairly local check: a stable-sort on the operands would be deterministic and local to a single operation.
================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:235
+ bfsOfOperand->key = (Twine(bfsOfOperand->key) + Twine("1") +
+ std::string(frontAncestor->getName().getStringRef()))
+ .str();
----------------
The string conversion seems unnecessary to me?
================
Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:271
+ for (Operation *operandDefOp : operandDefOps) {
+ OperandBFS *bfsOfOperand = new OperandBFS();
+ bfsOfOperand->pushAncestor(operandDefOp);
----------------
Is this a leak?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124750/new/
https://reviews.llvm.org/D124750
More information about the cfe-commits
mailing list