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