This is an archive of the discontinued LLVM Phabricator instance.

[Support] Move ParseResult from OpDefinition.h to LogicalResult.h
ClosedPublic

Authored by lattner on Apr 13 2022, 10:01 PM.

Details

Summary

This class is a helper for 'parser-like' use cases of LogicalResult
where the implicit conversion to bool is tolerable. It is used by the
operation asmparsers, but is more generic functionality that is closely
aligned with LogicalResult. Hoist it up to LogicalResult.h to make it
more accessible. This is part of Issue #54884

Diff Detail

Event Timeline

lattner created this revision.Apr 13 2022, 10:01 PM
Herald added a project: Restricted Project. · View Herald Transcript
lattner requested review of this revision.Apr 13 2022, 10:01 PM
lattner added inline comments.Apr 13 2022, 10:02 PM
mlir/lib/Tools/PDLL/AST/Diagnostic.cpp
18

Just FYI, this is required because pdll uses the same class name as mlir core.

mehdi_amini added inline comments.Apr 13 2022, 10:25 PM
mlir/include/mlir/Support/LogicalResult.h
88

Isn't there a parser header that could host this?
It is quite more specific and narrow than "LogicalResult". Alternatively what about its own header? Either ParseResult.h or more generically ParserUtils.h (but then again why not in include/mlir/Parser/? include/mlir/Parser/Utils.h seems fine?)

I'd like to hoist this whole file up to LLVM/Support. LLVM has lots of parsers, and doesn't have a unified system for building these. "ParseResult" is perhaps misnamed - it is really "LogicalResult that is implicitly convertible to bool"

I have concerned about implicit conversion to bool in the context of LogicalResult though, it was very intentional that LogicalResult does not convert to bool implicitly. We only carved out a specific exception for the parser because of this somehow pre-existing common pattern of chaining the calls, so it isn't a "misname": it was very intentional to keep this scoped and localized.

It is so small (10 lines of code) that I would have each of the LLVM parser defining their own ParseResult if they which to do so. But if you really want to have this in llvm/Support, then llvm/Support/ParseResult.h would do it as well?

Something else that shows in this patch is a "layering violation" where the mlir/IR/Diagnostics.h classes are now exposed here.

Something else that shows in this patch is a "layering violation" where the mlir/IR/Diagnostics.h classes are now exposed here.

I agree, I'll work on this.

I have concerned about implicit conversion to bool in the context of LogicalResult though, it was very intentional that LogicalResult does not convert to bool implicitly. We only carved out a specific exception for the parser because of this somehow pre-existing common pattern of chaining the calls, so it isn't a "misname": it was very intentional to keep this scoped and localized.

I don't understand your concern here. This patch isn't making LogicalResult convertible to bool (fwiw, River is also staunchly opposed to this). I'm pretty aware of this history here. This patch is just moving the ParseResult class into LogicalResult.h.

You are correct that we could define a new ParseResult.h next to LogicalResult, but I don't see how a 5 line class warrants its own header, and there is nothing "dangerous" about what the patch is proposing. I also don't see how putting this in a Utils.h is better.

Are you /opposed/ to this moving into LogicalResult.h or are you just discussing?

-Chris

lattner updated this revision to Diff 423275.Apr 16 2022, 9:39 PM

Move the implicit conversion from ParseResult.

It makes more sense to put these on InflightDiagnostic and Diagnostic,
which already have the corresponding conversion to LogicalResult.
Diagnostic.h already includes LogicalResult.h as well. This fixes a
layering issue.

lattner updated this revision to Diff 423276.Apr 16 2022, 9:43 PM

Update class comments, clarifying rationale and further dissuading people
from using it for general error handling.

I'm LGTM on having this moved to LogicalResult in general, having seen various copies or places using raw bool. Whether this is in LogicalResult.h or some ParseResult.h doesn't matter a lot to me (given how small it is at this point), the big thing to me is that the class has significant documentation that covers rationale/when it should be used/when it shouldn't. The important part to me is that any conversions to bool are well contained, and have documentation (I would have some concerns about this implicitness leaking outside of the places that it should).

mlir/lib/Tools/PDLL/AST/Diagnostic.cpp
18

Is this still required?

lattner updated this revision to Diff 423277.Apr 16 2022, 10:02 PM

Drop unneeded change River pointed out.

You're right, the PDL change isn't needed anymore, dropped from the patch.

Do the comments on the class work for you, or do you recommend anything more in particular?

rriddle accepted this revision.Apr 17 2022, 12:40 PM

LGTM from my point-of-view, but make sure that Mehdi is okay before pushing.

mlir/include/mlir/Support/LogicalResult.h
74

nit: Can we move this class to the bottom of the file?

81

parsering?

This revision is now accepted and ready to land.Apr 17 2022, 12:40 PM
mehdi_amini accepted this revision.Apr 17 2022, 1:04 PM

I have concerned about implicit conversion to bool in the context of LogicalResult though, it was very intentional that LogicalResult does not convert to bool implicitly. We only carved out a specific exception for the parser because of this somehow pre-existing common pattern of chaining the calls, so it isn't a "misname": it was very intentional to keep this scoped and localized.

I don't understand your concern here. This patch isn't making LogicalResult convertible to bool (fwiw, River is also staunchly opposed to this)

Right: sorry I added confusion to the discussion :)
You wrote that it is conceptually "LogicalResult that is implicitly convertible to bool" and I was just trying to say that I don't think we want ParseResult to be marketed that way and folks starting to use it widely outside of the context of the parser.

With the layering resolved by moving the conversions for InflightDiagnostic and Diagnostic, the current patch LGTM :)

cool, thank you both for the help!

This revision was landed with ongoing or failed builds.Apr 17 2022, 3:24 PM
This revision was automatically updated to reflect the committed changes.

Oh sorry, I didn't see River's additional comments. On it.