This is an archive of the discontinued LLVM Phabricator instance.

[mlir][mlir-lsp] Add a new C++ LSP server for MLIR named mlir-lsp-server
ClosedPublic

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

Details

Summary

This commits adds a basic LSP (Language Server Protocol, https://langserver.org) server for MLIR that supports resolving references and definitions. Several components of the setup are simplified to keep the size of this commit down, and will be built out in later commits. A followup commit will add a vscode language client that communicates with this server, paving the way for better IDE experience when interfacing with MLIR files.

The structure of this tool is similar to mlir-opt and mlir-translate, i.e. the implementation is structured as a library that users can call into to implement entry points that contain the dialects/passes that they are interested in.

Note: This commit contains several files, namely those in mlir-lsp-server/lsp, that have been copied from the LSP code in clangd and adapted for use in MLIR. This copying was decided as the best initial path forward (discussed offline by several stake holders in MLIR and clangd) given the different needs of our MLIR server, and the one for clangd. If a strong desire/need for unification arises in the future, the existence of these files in mlir-lsp-server can be reconsidered.

Depends On D100438

Diff Detail

Event Timeline

rriddle created this revision.Apr 13 2021, 7:56 PM
rriddle requested review of this revision.Apr 13 2021, 7:56 PM
mehdi_amini edited the summary of this revision. (Show Details)Apr 13 2021, 8:34 PM
mehdi_amini accepted this revision.Apr 13 2021, 9:09 PM

This whole design with MessageHandler, LSPServer, JSONTransport and how these are all intertwined isn't clear to me.
The individual class doc is not helping here, and I don't think improving these individual class is actually gonna solve it for me: I suspect I'd like to have some high-level description of how this fits. Maybe if expanding the classes themselves, the description could position them in the global scheme, and describe their role with respect to the others.

mlir/lib/Tools/mlir-lsp-server/LSPServer.cpp
18

Where is this used?

mlir/lib/Tools/mlir-lsp-server/MLIRServer.h
22

These are structs

mlir/lib/Tools/mlir-lsp-server/lsp/Transport.cpp
309
mlir/tools/mlir-lsp-server/mlir-lsp-server.cpp
20

return failed(... ?

This revision is now accepted and ready to land.Apr 13 2021, 9:09 PM
rriddle updated this revision to Diff 337909.Apr 15 2021, 2:38 PM
rriddle marked 4 inline comments as done.

rebase

jpienaar accepted this revision.Apr 19 2021, 5:24 PM

Nice, thanks

mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp
26

Could this method be reused below in line 31?

157

All the +1 and -1 are a bit messy, these probably don't change that much but seems easy to get wrong

160

containsPosition? (check is so non descript)

mlir/lib/Tools/mlir-lsp-server/lsp/Logging.cpp
34

No warning?

mlir/lib/Tools/mlir-lsp-server/lsp/Logging.h
24

Oooh, seems like could be something generally useful ;-)

mlir/lib/Tools/mlir-lsp-server/lsp/Protocol.cpp
62

Mmm, surprised this is a not somewhere more generally accessible (probably not common elsewhere in llvm even though common)

mlir/lib/Tools/mlir-lsp-server/lsp/Protocol.h
248

pos?

249

range? (2 chars extra and rng reminds of random number generator)

mlir/test/mlir-lsp-server/exit-with-shutdown.test
3

Is clangd expected?

rriddle updated this revision to Diff 339366.Apr 21 2021, 1:26 PM
rriddle marked 7 inline comments as done.

update

rriddle added inline comments.Apr 21 2021, 1:28 PM
mlir/lib/Tools/mlir-lsp-server/lsp/Logging.cpp
34

All of the warning style comments generally are either errors, or get wrapped into general logging. I haven't seen a situation that used a warning log level, but that is something that could change.

mlir/test/mlir-lsp-server/exit-with-shutdown.test
3

Ooops. It's not expected, but here it doesn't really matter.

This revision was landed with ongoing or failed builds.Apr 21 2021, 2:47 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Jan 2 2022, 10:01 PM
mlir/lib/Tools/mlir-lsp-server/lsp/Transport.cpp
277

Shouldn't you check the returned value here?