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

srishti-pm updated this revision to Diff 448682. srishti-pm marked 3 inline comments as done. srishti-pm added a comment. Addressed the final NITs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files:

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

srishti-pm updated this revision to Diff 448117. srishti-pm added a comment. Updated an outdated comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: mlir/include/mlir/Transforms/Commutativit

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

srishti-pm updated this revision to Diff 448094. srishti-pm marked 5 inline comments as done. srishti-pm added a comment. 1. Addressed all the comments. 2. Refactored the code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://revie

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

srishti-pm marked an inline comment as done. srishti-pm added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:110 + /// operand at a particular point in time. + DenseSet visitedAncestors; + Mogball wrote: > Since this is a set of p

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

srishti-pm marked an inline comment as done. srishti-pm added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:238 +// Stores the mapping between an operand and its BFS traversal information. +DenseMap operandToItsBFSMap; + Mo

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

srishti-pm updated this revision to Diff 446295. srishti-pm added a comment. Used the `stable_sort` function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: mlir/include/mlir/Transforms/Commutat

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

srishti-pm marked 4 inline comments as done. srishti-pm added inline comments. Comment at: mlir/lib/Transforms/Utils/CommutativityUtils.cpp:329-353 + assert(frontPosition >= 0 && frontPosition < bfsOfOperands.size() && + "`frontPosition` should be valid"); + unsigned po

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

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 b

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

srishti-pm updated this revision to Diff 441253. srishti-pm added a comment. Addressed of all Jeff's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: mlir/include/mlir/Transforms/Commuta

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

srishti-pm updated this revision to Diff 440848. srishti-pm marked 2 inline comments as done. srishti-pm added a comment. Fixed memory leak. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: mlir/i

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

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 accoun

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

srishti-pm updated this revision to Diff 440570. srishti-pm marked 6 inline comments as done. srishti-pm added a comment. Addressed the final comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 File

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

srishti-pm added a comment. In D124750#3587161 , @mehdi_amini wrote: > Right now I'm not yet understanding all of the algorithm (haven't spent > enough time on it), but I'm mostly concerned by the runtime cost of this > normalization. I understand you

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

srishti-pm updated this revision to Diff 437276. srishti-pm added a comment. Increasing pattern benefit + minor typo correction. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: mlir/include/mlir/

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

srishti-pm added a comment. Addressed most of the comments. A few remaining. 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-co

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

srishti-pm updated this revision to Diff 437249. srishti-pm marked 20 inline comments as done. srishti-pm added a comment. Addressing comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: mli

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

srishti-pm updated this revision to Diff 436281. srishti-pm added a comment. Minor changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: clang/docs/tools/clang-formatted-files.txt mlir/inclu

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

srishti-pm updated this revision to Diff 436274. srishti-pm added a comment. Addressed all the comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: clang/docs/tools/clang-formatted-files.txt

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

srishti-pm added a comment. Herald added a subscriber: bzcheeseman. @Mogball @mehdi_amini @jpienaar, sorry there haven't been any updates from my side here for the past 10 or so days. I had been busy in some other tasks. I have started working on this again. Repository: rG LLVM Github Monore

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

srishti-pm added a comment. **`Replying to the various comments given in this revision:`** **1. Regarding string manipulation:** > I need to look at the algorithm in more detail, but I'm not a fan of using a > string key. Concatenating strings to make compound keys is not very efficient > and

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

srishti-pm added a comment. > Thanks for improving the doc! Are you moving this to be used in > canonicalization next? > I think a first good step would be to make it a pattern and test it with a > pass that applies it in the greedy rewriter. I would land this first and then > try to enable thi

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

srishti-pm updated this revision to Diff 428502. srishti-pm added a comment. Fixing a minor typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: clang/docs/tools/clang-formatted-files.txt mlir

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

srishti-pm updated this revision to Diff 428498. srishti-pm added a comment. Improving the documentation of the functionality of this utility to make it more clear. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org

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

srishti-pm added a comment. > Why? Again your description of the sort is as follow: I think I need to update my commit summary and revision summary. I had thought that this was an intuitive way of explaining the utility. But, I understand that it is quite misleading. Repository: rG LLVM Git

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

srishti-pm added a comment. > Every time the algorithm would consider a commutative op, that is all the op, > it would recurse and try to re-sort the current slice, processing the same > ops over and over. Yes, exactly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

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

srishti-pm added a comment. > So: I understand that the producers should be sorted for a pattern to apply, > but our disconnect earlier is that my usual approach to see canonicalization > is to process an entire block/region, and as such we don't work on slices but > on operation in order until

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

srishti-pm added a comment. > So far you explained "what is canonicalization and why are we > canonicalizing", the same rationale applies to "push constant to the right" > that we do already in canonicalization, and this is exactly why I asked > before whether we could do what you're doing as a

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

srishti-pm added a comment. > My "why?" question is about canonicalization: could this be a > canonicalization and if so why / why not? This is an important thing to > answer before looking into the discussion below actually: I think, yes, it can be a canonicalization. I don't see why not. Re

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

srishti-pm updated this revision to Diff 428300. srishti-pm added a comment. Make the sorting strictly stable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: clang/docs/tools/clang-formatted-fil

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

srishti-pm added a comment. In D124750#3502295 , @mehdi_amini wrote: > You're telling me "what" while I'm actually more interested in the "why" here? I'm not sure what your question is, with a "why". Let me think about this a bit. I'll get back to you.

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

srishti-pm updated this revision to Diff 428240. srishti-pm added a comment. Within constant-like ops, removed the requirement for them being sorted alphabetically. Basically, all constants will be treated as equals by the sorting algorithm and it will not distinguish between, say, `arith.consta

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

srishti-pm added a comment. In D124750#3500748 , @mehdi_amini wrote: > Seems like this should be added to canonicalization? The "push constants to > the right hand side" is there already. I think this was not added to canonicalization because we wanted

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

srishti-pm updated this revision to Diff 427964. srishti-pm added a comment. Fixing a comment typo and enhancing the commit summary even further. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: c

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

srishti-pm updated this revision to Diff 426335. srishti-pm added a comment. Improving the commit summary Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124750/new/ https://reviews.llvm.org/D124750 Files: clang/docs/tools/clang-formatted-files.tx

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

srishti-pm created this revision. Herald added subscribers: sdasgup3, wenzhicui, wrengr, Chia-hungDuan, dcaballe, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, antiagainst, shauheen, rriddle, mehdi_amini, mgorny.