This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a new AsmParserState class to capture detailed source information for .mlir files
ClosedPublic

Authored by rriddle on Apr 13 2021, 7:55 PM.

Details

Summary

This information isn't useful for general compilation, but is useful for building tools that process .mlir files. This class will be used in a followup to start building an LSP language server for MLIR.

Depends On D100437

Diff Detail

Event Timeline

rriddle created this revision.Apr 13 2021, 7:55 PM
rriddle requested review of this revision.Apr 13 2021, 7:55 PM
mehdi_amini accepted this revision.Apr 13 2021, 8:44 PM
mehdi_amini added inline comments.
mlir/lib/Parser/AsmParserState.cpp
14
This revision is now accepted and ready to land.Apr 13 2021, 8:44 PM
jpienaar accepted this revision.Apr 15 2021, 9:52 AM

Nice, thanks. And so only overhead here is checking if null in regular execution?

mlir/include/mlir/Parser.h
83

Could you document AsmParserState usage here? (e.g., when should it be passed in, when does it not need to, what are the considerations while doing so)

110

Same

mlir/include/mlir/Parser/AsmParserState.h
7

File description?

13

Where is this used here?

25

blob? textual representation? Or is the intention that binary format later would use the same? (I'm assuming not as it is Asm*State)

41

OOC is the order required here? And/or is quick lookups of uses needed (a la sets) or just iterating over?

59

name? is this the "%id" identifier of the operation?

87

Could this be expanded? E.g., "Access State" seems very generic

mlir/lib/Parser/AsmParserState.cpp
106

So +1 is sufficient here, do we need to consider regular doubling in size to avoid degenerate resize cases?

128

Why reverse here?

mlir/lib/Parser/Parser.cpp
589

(up to you) Should this perhaps me made into a lamba here? E.g., return maybeRecordLocation(result); so that it is more difficult to forget. Probably we won't have an explosion of returns here ;)

787

Reserve space outside loop?

1852

Do we only need this for its location?

rriddle updated this revision to Diff 337908.Apr 15 2021, 2:38 PM
rriddle marked 11 inline comments as done.

rebase

rriddle added inline comments.Apr 15 2021, 2:40 PM
mlir/include/mlir/Parser/AsmParserState.h
7

We haven't really been adding file descriptions that basically just say "read the class description".

13

Thanks for the catch.

25

Using this for binary format wouldn't be useful, so switched wording to "MLIR textual format string"

41

Order generally isn't important, but efficient lookup is. After the initial vscode server is up, I'd like to look into more efficient encodings for here when we have larger .mlir files being processed. This is kept simple for now.

59

This is literally the name of the operation, i.e. the "foo.op". SSA value identifiers are tracked via the result groups.

87

What do you have in mind? This code section is for accessing the state, not populating it.

mlir/lib/Parser/AsmParserState.cpp
106

This is kind of relying on SmallVector::grow to have the right behavior, instead of reinventing it here. SmallVector::grow generally grows by 2 * oldCapacity + 1 when the new size is >= the current capacity. If we hit a really bad degenerate case, we could try to restructure the parser to allow for reserving the size here upfront.

128

The result groups are setup in increasing increments, e.g.:

// This operation:
%result, %result1:2, %result2 = ...

// Has the following result groups:
[0, {}], [1, {}], [3, {}]

The result group of value is the first group from the end that has a smaller/equal start point.

mlir/lib/Parser/Parser.cpp
1852

Yep. parseToken already does the equivalent of getToken()(which is basically free), so the only downside is increased lifetime of the variable.

This revision was landed with ongoing or failed builds.Apr 21 2021, 2:46 PM
This revision was automatically updated to reflect the committed changes.