[PATCH] D78788: [MLIR] Modify Analysis op conversion mode to emit errors for all unlegalizable operations.

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

In D78788#2001142 <https://reviews.llvm.org/D78788#2001142>, @lucyrfox wrote:

> Took a first pass at this, but I'm open to different ways of approaching this change. At I've written it now, the basic idea of the Analysis conversion is "we're performing a partial conversion and emitting warnings along the way for any ops that would fail to legalize in a full conversion". Success is returned as long as the partial conversion succeeds. I did it this way because returning failure outright means that the partially converted IR doesn't get emitted, which is useful for debugging.


Before answering, please note that we do not base these decisions based on debugging. What the result is depends on what the user intends to do with the result.

> However, an implication of this is that if the legalization //did// fully succeed, we don't know that based on the Analysis conversion. As a result, I foresee a lot of users running an Analysis conversion immediately followed by a Full conversion, thus incurring an unnecessary pass over the IR. So ideally, this would return success/failure for the full conversion while still outputting the partially converted IR in the failure case.

Seems like the user can easily tell if the conversion was completely successful by checking if the set of unlegalized operations is empty?

> What do you think?

We can change analysis conversion to whatever we want, but the main question is what are you doing with the information? What happens after the analysis conversion and what do you want to convey? We may end up with different types of analysis conversions based on the answer to that question. For example, the original envisioning for this was that a user would performs analysis conversions targeting different backends to detail which parts of the subgraph were really supported for a specific target. In that type of mode you don't want to do any conversions, because you want to splice the original graph based on the analysis. It seems that the use case you are targeting is different. For your use case, what is the pipeline that you are envisioning and how does an analysis conversion fit into it?



================
Comment at: mlir/lib/Transforms/DialectConversion.cpp:1575
+      /// An analysis conversion emits any errors that would occur if a full
+      /// conversion were attempted, but should continue on without failure
+      /// (i.e. performing a partial conversion).
----------------
I'm not sure we always want warnings here, seems like more of a DEBUG log. If a user is relying on an analysis conversion to help slice code that is supported by different backends, they may not care that certain operations weren't converted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78788/new/

https://reviews.llvm.org/D78788