This is an archive of the discontinued LLVM Phabricator instance.

[AsmParser] Introduce a new "Argument" abstraction + supporting logic
ClosedPublic

Authored by lattner on Apr 28 2022, 5:26 PM.

Details

Summary

MLIR has a common pattern for "arguments" that uses syntax
like %x : i32 {attrs} loc("sourceloc") which is implemented
in adhoc ways throughout the codebase. The approach this uses
is verbose (because it is implemented with parallel arrays) and
inconsistent (e.g. lots of things drop source location info).

Solve this by introducing OpAsmParser::Argument and make addRegion
(which sets up BlockArguments for the region) take it. Convert the
world to propagating this down. This means that we correctly
capture and propagate source location information in a lot more
cases (e.g. see the affine.for testcase example), and it also
simplifies much code.

Diff Detail

Event Timeline

lattner created this revision.Apr 28 2022, 5:26 PM
lattner requested review of this revision.Apr 28 2022, 5:26 PM

This also fixes the source location parsing bug that @mehdi_amini mentioned a couple days ago.

lattner updated this revision to Diff 425940.Apr 28 2022, 5:40 PM

Remove now-constant argument.

rriddle accepted this revision.Apr 28 2022, 10:38 PM
rriddle added inline comments.
mlir/include/mlir/IR/OpImplementation.h
1226
1234
1238

I understand the intention of how these defaults are chosen, but right now it feels like the current uses overwhelmingly skew the other way (i.e. false).

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1479
mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
779
mlir/lib/Dialect/SCF/SCF.cpp
407–408
This revision is now accepted and ready to land.Apr 28 2022, 10:38 PM
lattner updated this revision to Diff 425975.Apr 28 2022, 10:53 PM

Update flang

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 10:53 PM
lattner updated this revision to Diff 425976.Apr 28 2022, 10:59 PM
lattner marked an inline comment as done.

Incorporate improvements from River, still need to confirm sense of the bools in parseArgument

lattner marked 2 inline comments as done and an inline comment as not done.Apr 28 2022, 11:06 PM
lattner added inline comments.
mlir/include/mlir/IR/OpImplementation.h
1238

Perhaps. I could totally swap their sense to default to false, and that would give a more "opt into the behavior you want" mode which would avoid an error of omission. Do you think that's the right call?

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1479

Nice, I tried llvm::drop_front and that didn't work, this did the trick thanks!

lattner updated this revision to Diff 425977.Apr 28 2022, 11:06 PM

Two other minor comment changes I missed from River's review.

rriddle added inline comments.Apr 28 2022, 11:11 PM
mlir/include/mlir/IR/OpImplementation.h
1238

Given that every use we have so far wants at least one of these to be false, feels like that is the right "default". That being said, we have so few of these in general, it doesn't matter much to me. I can see pros/cons to either, I'll leave the final call up to you.

Yep, you're right, it is also much less common to have attributes, and no one wants attributes without a type. I'll flip it, thanks for the suggestion!

lattner updated this revision to Diff 426093.Apr 29 2022, 9:30 AM

Flip the default on allowType/allowAttr

This cleans up most of the callsites in a very nice way, and
is more "opt in" rather than "opt out". Thanks to River for
the suggestion!

tpopp added a subscriber: tpopp.May 2 2022, 12:24 AM

Hello, why were parseOptionalAssignmentListWithTypesand parseAssignmentListWithTypes removed? This seems to be a common pattern in MLIR where functionality is provided but untested and unused inside of core and then freely removed while breaking users. If this is the intended progression of the API, a deprecation would be appreciated, so people can raise concerns that used functionality is being deleted. If this being freely removed because users of the code outside of the LLVM repository don't impact this decision, it really indicates that MLIR core needs to start providing some form of a stable API that people can rely on rather than the current (small) chaos caused by many changes.

Hello, why were parseOptionalAssignmentListWithTypesand parseAssignmentListWithTypes removed?

This was simply dead code in the tree. I'm sorry I didn't realize other people were using it, I didn't mean to break anyone intentionally. There is a simple replacement with parseArgument and parse comma separated list, though we could add back if it would help with your internal build and integration for awhile.

-Chris

tpopp added a comment.May 4 2022, 7:00 AM

Hello, why were parseOptionalAssignmentListWithTypesand parseAssignmentListWithTypes removed?

This was simply dead code in the tree. I'm sorry I didn't realize other people were using it, I didn't mean to break anyone intentionally. There is a simple replacement with parseArgument and parse comma separated list, though we could add back if it would help with your internal build and integration for awhile.

-Chris

There were no large worries for our internal work. I essentially copied the code to the one user which was mlir-hlo's gml.st_loop: https://github.com/tensorflow/mlir-hlo/blob/master/tests/Dialect/gml_st/ops.mlir#L71

My comment was largely to push for a stricter process with seemingly dead code by commenting on commits when I see it.

Regarding the assignment list with types, I have only one example of this use case currently, so it is debatable if it deserves to be in core. I would argue for keeping such functionality, as I think the format in gml.st_loop will be more common with upstream users that represent large structured pieces of code for novel machine learning related hardware, but I don't think it's crucial due to being only a single use currently. Do you have any thoughts on the matter?

I don't think it should exist in core if it only has one user. In fact, I'd like to see the AsmParser stuff significantly refactored to be more modular and orthogonal. Most of this stuff exists because we didn't have parseCommaSeparatedList which defines away the majority of the utility of these macro helpers in the first place. See e.g. this patch:
https://reviews.llvm.org/D124791