Page MenuHomePhabricator

[mlir] Refactoring a few Parser APIs
ClosedPublic

Authored by sdasgup3 on Nov 11 2021, 2:38 PM.

Details

Summary

Refactored two new parser APIs parseGenericOperationAfterOperands and
parseCustomOperationName out of parseGenericOperation and parseCustomOperation.

Motivation: Sometimes an op can be printed in a special way if certain criteria
is met. While parsing, we need to handle all the versions.
parseGenericOperationAfterOperands is handy in situation where we already
parsed the operands and decide to fall back to default parsing.

parseCustomOperationName is useful when we need to know details (dialect,
operation name etc.) about a parsed token meant to be an mlir operation.

Diff Detail

Event Timeline

sdasgup3 created this revision.Nov 11 2021, 2:38 PM
sdasgup3 requested review of this revision.Nov 11 2021, 2:38 PM
rriddle requested changes to this revision.Nov 11 2021, 2:47 PM
rriddle added inline comments.
mlir/lib/Parser/Parser.cpp
313–316

This feels like much too specific of an API, what is the exact contract you are going for here? Trying to inject pre-parsed bits into the generic operation parser?

352–357

Why does this need to expose the dialect and abstract operation?

This revision now requires changes to proceed.Nov 11 2021, 2:47 PM
sdasgup3 updated this revision to Diff 386683.Nov 11 2021, 4:35 PM
  1. Updating D113719: [mlir] Refactoring a few Parser APIs #

Fixed an issue with the API CustomOpAsmParser::parseCustomOperationName exposing
the AbstractOperation.

sdasgup3 added inline comments.Nov 11 2021, 4:49 PM
mlir/lib/Parser/Parser.cpp
313–316

Thanks for the comments!

Most of the custom printers prints the op-name and operands differently than the generic printer. e.g. the op-name w/o double quotes and operands w/o parenthesis. The op specific custom-parsers parses those accordingly. The above API is useful to parse beyond the operands if the op-specific parser decides to fall back to generic parsing.

One such scenario is provided in the example TestDialect, where after the operands are parsed, the custom-op parser can fallback to generic parsing and calls the above API. That way we can avoid calling the individual parsing methods like parseRegion, resolveSSAUse etc. and instead reuse a part of parseGenericOperation.

I find this API handy while implementing the "pretty-printing for the mhlo reduce-op".

Please let me know your thought?

352–357

Thanks for pointing out!
I agree that it does not make sense for OpAsmParser::parseCustomOperationName to expose the internal artifacts,
Although OperationParser::parseCustomOperationName needs to expose such internal details as it's caller OperationParser::parseCustomOperation needs that.

sdasgup3 updated this revision to Diff 386985.Nov 12 2021, 4:35 PM
  1. Updating D113719: [mlir] Refactoring a few Parser APIs #

Fixing build failures.

rriddle added inline comments.Nov 14 2021, 11:28 PM
mlir/lib/Parser/Parser.cpp
313–316

Right, I think passing in pre-parsed things makes sense, but I would like to avoid having N parseGenericOperationAfter* methods for each combination of components. Could we just expose one method that allows for optionally taking all of the components, and we just parse the things not explicitly passed in?

352–357

Right, what we expose here doesn't need to be the same as what the internal parser needs. Here we care more about the API being exposed. Like, I think here we could even return FailureOr<OperationName> if that was desirable.

sdasgup3 updated this revision to Diff 387892.Nov 17 2021, 3:51 AM
  1. Updating D113719: [mlir] Refactoring a few Parser APIs #

Refactored the APIs as per the following review comments.

  1. The API parseGenericOperationAfterName allows for optionally taking all of

the components, and we just parse the things not explicitly passed in.

  1. parseCustomOperationName returns FailureOr<std::pair<std::string, Dialect

*>> to accomodate the failure scenario.

sdasgup3 marked 2 inline comments as done.Nov 17 2021, 3:54 AM
rriddle added inline comments.Nov 18 2021, 10:45 AM
mlir/include/mlir/IR/OpImplementation.h
913

Can you switch this to OperationName now? A recent refactoring has made that a better choice.

931–935

Can you add llvm::None to reduce the number of forced parameters? (We could also possibly switch to a struct if it gets too crazy in practice, but this is fine for now).

mlir/lib/Parser/Parser.cpp
320–324

Can we add default params(llvm::None) for these?

1003

You shouldn't need the .hasValue() on any of these.

1058–1092

Can we just assign parsedFnType in the first if, and sink the rest of the logic out of the conditional? (i.e. similar to operandUseInfo above) e.g.

if (!parsedFnType) {
   ...
   parsedFnType = fnType;
}

result.addTypes(parsedFnType->getResults());

// Check that we have the right number of types for the operands.
...
1205–1212

Can you add a comment here? Realistically we shouldn't need this, we should make SSAUseInfo a typedef of OperandType. (You don't need to do that, I can do that in a followup).

1644

Can you add a move here to avoid copying the string? Either that or switch this to StringRef?

mehdi_amini added inline comments.Nov 18 2021, 11:21 PM
mlir/lib/Parser/Parser.cpp
352–354

The doc does not match the implementation: this isn't a pair but a tuple.

(I suspect that returning an FailureOr<OperationName> as River mentioned should be enough anyway)

1195

Missing std::move here as well to avoid string copy

sdasgup3 updated this revision to Diff 388408.Nov 19 2021, 12:49 AM
sdasgup3 marked 5 inline comments as done.
  1. Updating D113719: [mlir] Refactoring a few Parser APIs #

Addressed all the comments except the one related to leveraging a recent change
https://reviews.llvm.org/D114049. Waiting of the change to sync in the
repository I am working on.

sdasgup3 added inline comments.Nov 19 2021, 12:50 AM
mlir/lib/Parser/Parser.cpp
1058–1092

Thanks for pointing this out as I am also not happy with this piece of code.

As you found that in both if else, we have some repeated code after the comment "// Check that we have the right number of types for the operands." .
The only difference is the call to emitError: In the if part, we are leveraging the extra location information typeLoc, which is not available if the argument parsedFnType is explicitly provided.
I wish if I could use some location info to use in the else part as well. That way we can refactor-out the then-becoming-common-code out of the conditional.

Let me know about any suggestions.

1205–1212

I agree with you and added the comment.
Actually these the two types: one internal to parser implementation (OperationParser::SSAUseInfo {name, number, loc}) and the other exposed to clients of parser (OpAsmParser::OperandType {location, name , number}), both have the same members but in different order. As you said, It would be much cleaner if one could be the alias of other.

rriddle added inline comments.Nov 19 2021, 12:21 PM
mlir/lib/Parser/Parser.cpp
1058–1092

I think for now you could just use result.location as the location in the else.

sdasgup3 updated this revision to Diff 388648.Nov 19 2021, 3:27 PM
sdasgup3 marked 5 inline comments as done.
  1. Updating D113719: [mlir] Refactoring a few Parser APIs #

Changed the return type of pasreCustomOperationName to OperationName.

mlir/lib/Parser/Parser.cpp
1058–1092

Done.
Just for FYI:

I used mlir::emitError(mlir::Location, ...) version instead of the Parser::emitError(llvm::SMLoc, ...) to accommodate the fact that result.location is of type mlir::Location. The decision is made because

  1. There is no conversion routine available from llvm::SMLoc to mlir::Location and
  2. There exist no version like Parser::emitError(mlir::Location. ...)
sdasgup3 marked an inline comment as done.Nov 22 2021, 8:07 AM
sdasgup3 added inline comments.
mlir/lib/Parser/Parser.cpp
1195

This is not applicable after the recent code change.

rriddle accepted this revision.Nov 22 2021, 12:21 PM
rriddle added inline comments.
mlir/include/mlir/IR/OpImplementation.h
910–912

nit: I don't think the description of OperationName is necessary here.

mlir/lib/Parser/Parser.cpp
1678

opNameInfo.getIdentifier().c_str()?

That would avoid a string copy.

1682

Can we pass in the OperationName (opNameInfo) here instead of opName? (it's just going to re-lookup the OperationName otherwise).

This revision is now accepted and ready to land.Nov 22 2021, 12:21 PM
sdasgup3 updated this revision to Diff 389024.Nov 22 2021, 1:17 PM
sdasgup3 marked 3 inline comments as done.
  1. Updating D113719: [mlir] Refactoring a few Parser APIs #

Addressed the review comments.

mlir/lib/Parser/Parser.cpp
1678

Agreed! changed it to opNameInfo.getIdentifier().data()

This revision was landed with ongoing or failed builds.Nov 22 2021, 10:13 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Nov 22 2021, 10:13 PM
mlir/test/lib/Dialect/Test/TestDialect.cpp
747

Please avoid auto when the type isn't obvious from the context (or not spell-able/readable).

sdasgup3 marked an inline comment as done.Nov 23 2021, 3:33 AM
sdasgup3 added inline comments.
mlir/test/lib/Dialect/Test/TestDialect.cpp
747

Agreed. Thanks for the fix.