River707 wrote: > > The only very high level conern I have is the use and need of > > `InsertionPoint::after`. As I understand, this is a not-yet-implemented > > optimization for the future for the purpose of reusing materializations by > > calculating a better insertion point where it may dominate more operations > > which might need exactly that materialization after pattern application as > > well. > > Yes, that's correct. But this optimization is already implemented. And it is > actually quite deeply ingrained into the dialect conversion driver. > > SSA values are not replaced directly, but in a delayed fashion. The > replacement value is stored in the `ConversionValueMapping` (a kind of > `IRMapping`). We store not only user-specified replacement values but also > materializations in there. The data structure is kind of a linked list: for > an original value, we store a list of replacement values: the specified > replacement value and materializations to a different type. In the "finalize" > phase, the driver goes through that list to find a > replacement/materialization value with the correct type. We cannot store more > than one replacement/materialization value with the same type; the driver > would just pick the first one that matches. So we always reuse. > > Note: Computing an insertion point for a 1:1 materialization (or a 1:N block > signature conversion) is much easier because there is only one SSA value (or > one block). That's why computing the insertion point was trivial until now. > > > Is the current complexity of calculating the insertion point (and > > potentially calculatin the post-dom tree, a O(n log n) operation) worth the > > gain of the optimization? I am thinking that the better insertion point > > insertion logic should probably be post-poned until we can measure its > > effectivness (and avoid the risk of a premature optimization) and have > > something simpler and working that does not worsen the status-quo for now > > instead. > > This PR also makes the `DominanceInfo` argument to `InsertPoint::after` > optional in case of a single SSA value. (And no `DominanceInfo` is passed in > that case.) For the most frequent case of 1:1 replacements, we do not compute > a dominator tree at all. (And we are not doing any extra work.) > > In case of N:1 materializations, the implementation uses `DominanceInfo` and > we create it during `ConversionPatternRewriter::replaceOpWithMultiple`. > Unfortunately, it is not safe to reuse the same `DominanceInfo` object > because a pattern could have made IR changes that invalidate the dominator > tree. > > However, I believe `DominanceInfo` is quite "cheap" to use. The dominator > tree is built lazily and it is built on a per-region basis. E.g., creating a > new `DominanceInfo` and querying dominance for two ops in the same region > will just build the dominator tree for that region (and only if the ops are > in different blocks). In case of two ops from different regions (or different > blocks in the same region), the implementation will find the common ancestors > (in the same region) and then compute the dominator tree only for that region. > > Long term, the `ConversionValueMapping` is going to disappear with the > One-Shot Dialect Conversion. As part of that, I'm also thinking of removing > the "materialization caching" mechanism and just building duplicate > materializations (in the form of `unrealized_conversion_cast`, i.e., not > calling the callback). These can then be CSE'd before calling the > materialization callback. The CSE-ing will require `DominanceInfo`, but the > same `DominanceInfo` can be reused for the entire dialect conversion because > at this point of time we are done with pattern applications.
One question I have here is why not just insert right before the operation that is being replaced? What is the need for trying to insert after one of the values? https://github.com/llvm/llvm-project/pull/115816 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
