Hi All,
One of the input dialects to our compiler is a custom dialect (which we don’t own) that has verifiers for some op arguments.
We’d like to gradually convert it to another dialect (which we do own).
However, the input dialect’s verifiers prevent us from doing it gradually in a way that seems reasonable for us, and I’m wondering if there’s a good way around it.
We have a similar issue with an op that has a HasParent trait, which also prohibits us from moving it during conversion.
From my perspective, verifying the IR is important during its construction, but if I want to gradually change it, those verifiers do more harm then good.
Am I missing something here? I saw there’s a way to disable the PassManager’s verification (using passManager.enableVerifier(false)), but I don’t want to do it for all passes, just for the conversion ones.
Thanks in advance,
Yotam
I guess I can define 2 PassManagers, one with verification and one without. It feels like I’m doing something wrong though, so your inputs will be much appreciated as I’m fairly new to MLIR.
While it is possible, it makes debugging more tricky and flagging invalid edits delayed/difficult to act on (which includes bisecting). During a pass though, it is often invalid. So many times its handled all inside a pass. In your case it sounds like, its not easy to handle in one pass, is that correct?
Doing multiple PassManager with verification disabled is one option, the other is doing something like the CompositePass which is really similar to previous executionally (its effectively a pass that invokes a pass pipeline, but at top level/for debugging/reproducers/bisection its treated as unit), or expanding all into helpers and calling one after the other in the same pass (there is nothing stopping one from doing multiple rewrite drivers in the same pass, it just makes the passes fixed together, which may be whats intended as they aren’t meaningful independently, it could complicate testing the individual parts if care is not taken).
This looks like a design issue to me with either the verifiers (are they local or are they looking through the SSA chains?), or the way you are thinking about the conversion.
You can construct invalid IR, it just shouldn’t exit the pass boundary, maybe you can provide some more concrete examples?
You mentioned the HasParent traits, this is an example of an op that must be converted alongside the parent.
Sorry for sidetracking, but this has been a source of confusion to me for a while.
Shouldn’t we require every pattern to generate valid IR?
Good point @banach-space!
The mindset has evolved on this over time I believe. In early days it was very common to create invalid IR across patterns. We were also using the greedy rewriter to do DialectConversion, and even after we got conversion pattern, not all kind of conversion were supported and required to do modification outside of the rewriter, sometimes breaking the IR validity temporarily.
However I don’t believe this is common anymore. @matthias-springer (I think?) implemented an assertion for verifying the IR between individual pattern applications that is enabled behind the CMake -DLLVM_ENABLE_EXPENSIVE_CHECKS=ON option. It fires in a few places in tree still last time I checked, so it’s not something where we’re really there yet.
Most, possibly all, logic in the op class and related utilities assumes the op has passed the verifier. Note that the verifier includes conditions on things like the number of operands, the existence of attributes with a certain name and type. For example, when you call op.getOperandWithSomeName(), it is in fact doing op->getOperand(42) without checking whether there are enough operands because, in valid IR, there will be.
I’d also differentiate op-specific verifiers from “core IR” verifiers for things like dominance and proper region nesting.
Working with invalid IR is possible but quite error-prone as the code must know exactly which invariants are broken and which can be relied upon. This typically means the fixups of invalid IR use lower-level APIs such as direct lists of operands or use-def chains rather than specific ops, which is uncommon in patterns.
Based on the above, it is technically sufficient that the IR maintains the invariants expected by the rewriter driver and all applicable patterns. Certainly, this is automatic if all patterns generate valid IR.
Indeed, it’s usually better to convert the parent op + all terminators within its blocks + perform argument conversion on its regions simultaneously. There’s a talk somewhere in the archives on setting up dialect conversions.
I don’t think we did. The initial commit introducing it specifically contrasts it with the greedy driver: Generic dialect conversion pass exercised by LLVM IR lowering · llvm/llvm-project@6d37a25 · GitHub .
I don’t quite get the contradiction? We must be talking about something different here.
We built the DialectConversion to address the shortcoming of using the greedy rewriter, but the greedy rewriter was used for lowering: first because before the commit you mention there wasn’t much other option, later because the DialectConversion framework had limitations, and then for the wrong reasons (inertia, lack of knowledge, etc.).
Even right now in the codebase, git grep applyPatternsGreedily returns a lot of instances of lowering passes that still use it (including passes in mlir/lib/Conversion/...).
I think this is mainly because as long as you don’t need to convert types, the GreedyRewriter has been doing an OK job at it.
I think we’re digressing a bit here, not clear this would change anything to the current topic (or my answer to the topic) anyway.
I’m trying to clarify the potential misunderstanding on the origin and intention of the DialectConversion facility. There may be a conflation between the process of lowering / converting dialects, and the DialectConversion facility. Apparently, some folks just look through forum posts to get this information rather than asking, including older (I saw people quote 2-3-year old posts) and vaguely relevant topics. It would be a shame if they got incorrect information from this one.
I wrote DialectConversion initially. The goal was to implement standard-to-LLVM dialect conversion that required changing types. At the time, (not yet) upstream MLIR didn’t have type conversion at all and not much of the lowering. There was a dedicated pass converting (what would become) affine loops to CFG, but it was fully separate as it required operating on different IR constructs as this was prior to regions and even value kind unification. Even with River’s further changes to DialectConversion to use the same base pattern class as the other driver, it was never greedy. There were changes bypassing the rewriter leading to bugs, many due to the unificaiton, but those were most often violating “core” assumptions like ops still using values defined by deleted operations.
Some lowerings / dialect changes to indeed use the greedy driver. Primarily stemming from the Vector dialect and friends, because of the multi-stage lowering as demonstrated here.
Indeed, this won’t change much to the answer except maybe for the rationale about IR validity at pattern boundary vs. pass boundary. It is nice to get the record straight though.