RewriterBase is the proper builder to use so one can listen to IR modifications (i.e. not just creation).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
What's the reason for this? I've been "cleaning up" some code in the past by using OpBuilder instead of PatternRewriter/RewriterBase/... when possible. The reason for this was better compatibility (both builders and rewriters can be used) and code documentation (when the function decl takes an OpBuilder, the caller can be sure that the function does only create new IR and never deletes/modifies existing IR).
If using RewriterBase is actually a best practice, we should document it somewhere, otherwise, somebody may come along with the same idea as me in a few months and undo your change.
I dont see a strong reason to do this. My thumb rule has been whenever there is need to erase operations use RewriterBase. If this is just building IR, OpBuilder usage is more stable IMO and more widely applicable.
There are multiple reasons IMO.
https://reviews.llvm.org/D137690 significantly increases the capabilities of the vectorizer by adding masking, as part of that the following code is introduced:
Operation *maskOpTerminator = &maskOp.getMaskRegion().front().back(); for (auto &en : llvm::enumerate(opToMask->getResults())) en.value().replaceAllUsesExcept(maskOp.getResult(en.index()), maskOpTerminator);
This is an enormous footgun and it needs to go through a RewriterBase updateRootInPlace which will permeate through the whole file all the way to the top.
Using a RewriterBase also encourages proper use of the debugging infra and avoids gross workarounds such as IRRewriter(b).notifyMatchFailure().
Lastly, from a consistency and cognitive overhead perspective, using a single thing is simpler: if you write a transform use RewriterBase, if you write a builder use OpBuilder.
Otherwise the rule of thumb is: use a RewriterBase *in the whole call stack to any leaf that modifies IR* (either erase, inplace update or update of uses) otherwise it may be ok to use OpBuilder but you may have to modify the whole call stack if in the future IR modifications occur (otherwise very subtle bugs may appear).
Such code should be flagged during review (as you did). I've seen people using Value::replaceAllUses despite having a RewriterBase readily available in the same function, so whether the function takes an OpBuilder or a RewriterBase may not make a difference here.
Lastly, from a consistency and cognitive overhead perspective, using a single thing is simpler: if you write a transform use RewriterBase, if you write a builder use OpBuilder.
Some helper functions in Transforms/Vectorization.cpp are builders, e.g., buildVectorWrite.
Really this should be type safe and not best effort hope reviewers catch it .. we should make these private, expose to a very limited set of friends and force everything else through RewriterBase..
The current situation is just terrible.
Lastly, from a consistency and cognitive overhead perspective, using a single thing is simpler: if you write a transform use RewriterBase, if you write a builder use OpBuilder.
Some helper functions in Transforms/Vectorization.cpp are builders, e.g., buildVectorWrite.
Good point, some of these need to move indeed.
+1 to using notifyMatchFailure. That should be probably even moved out of RewriterBase . Its a good debugging tool irrespective of this change.
Lastly, from a consistency and cognitive overhead perspective, using a single thing is simpler: if you write a transform use RewriterBase, if you write a builder use OpBuilder.
Otherwise the rule of thumb is: use a RewriterBase *in the whole call stack to any leaf that modifies IR* (either erase, inplace update or update of uses) otherwise it may be ok to use OpBuilder but you may have to modify the whole call stack if in the future IR modifications occur (otherwise very subtle bugs may appear).
I was writing an explanation as to why this shouldnt be changed, but actually realized those explanations dont hold water. This looks good to me.
Thanks for the feedback! Please note that some of us come from LLVM where builders and UD chain manipulation is the daily basis :). Not sure I totally agree with the level of usage restriction suggested for some of these methods but I'm happy to try to do the same with a rewrite. Note, though, that this code is not doing a replacement but it's building a new mask operation and moving another existing operation into the mask operation region. Can we do that with updateRootInPlace?
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp | ||
---|---|---|
1427 | s/b/rewriter? (+readability). I think it's important to state that it's a rewriter and not a builder in the code. |
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp | ||
---|---|---|
1427 | +1, thanks! |
s/b/rewriter? (+readability). I think it's important to state that it's a rewriter and not a builder in the code.