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