This is an archive of the discontinued LLVM Phabricator instance.

[mlir][PDLL] Add an initial language server for PDLL
ClosedPublic

Authored by rriddle on Mar 13 2022, 12:47 AM.

Details

Summary

This commits adds a basic language server for PDLL to enable providing
language features in IDEs such as VSCode. This initial commit only
adds support for tracking definitions, references, and diagnostics, but
followup commits will build upon this to provide more significant behavior.

In addition to the server, this commit also updates mlir-vscode to support
the PDLL language and invoke the server.

Diff Detail

Event Timeline

rriddle created this revision.Mar 13 2022, 12:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 12:47 AM
rriddle requested review of this revision.Mar 13 2022, 12:47 AM
rriddle updated this revision to Diff 414998.Mar 13 2022, 10:15 PM
rriddle edited the summary of this revision. (Show Details)

(half scan, could you address clang-format ones too?)

mlir/include/mlir/Tools/mlir-pdll-lsp-server/MlirPdllLspServerMain.h
1

I find MLIR PDLL a bit overkill (mlir-pdll-lsp-server/MlirPdllLspServerMain.h too but lets skip that for now :) ), I think just PDLL should suffice. But up to you.

mlir/lib/Tools/PDLL/AST/Nodes.cpp
66

The others are in alphabetical order (per grouping), could we order these such too?

80

"what is the order of these visitImpl members"-snare triggered :) (could we we have them in the same order as you have above?)

mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.cpp
89

0.0.1 ?

mlir/lib/Tools/mlir-pdll-lsp-server/MlirPdllLspServerMain.cpp
26

Mmm, does PDLL files proper also use this? Or is this for tests of PDLL files?

41

Does this affect what is logged in IDEs/vscode's dash too?

mlir/lib/Tools/mlir-pdll-lsp-server/PDLLServer.cpp
30

Seems like this could be in the generic LSP part of the world.

jpienaar accepted this revision.Mar 14 2022, 3:52 PM
jpienaar added inline comments.
mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.h
23

Seems like this class could be moved to the CPP file and here just

LogicalResult runLspServer(...)

could be exposed.

mlir/lib/Tools/mlir-pdll-lsp-server/PDLLServer.cpp
83

Could you add a TODO/FIXME?

mlir/test/mlir-pdll-lsp-server/definition-split-file.test
1

Why strict-whitespace? AFAIK this shouldn't be load bearing

mlir/tools/mlir-pdll-lsp-server/mlir-pdll-lsp-server.cpp
15

Should asMainReturnCode be hoisted out somewhere generally to be useable here?

mlir/utils/vscode/src/configWatcher.ts
49

Nit: I'd make this mlir_server_path to be explicit

This revision is now accepted and ready to land.Mar 14 2022, 3:52 PM
rriddle updated this revision to Diff 416678.Mar 19 2022, 3:37 AM
rriddle marked 10 inline comments as done.
rriddle added inline comments.Mar 19 2022, 3:37 AM
mlir/lib/Tools/mlir-pdll-lsp-server/LSPServer.h
23

Oooh, great suggestion. Done. I'll update the MLIR server in an NFC followup.

mlir/lib/Tools/mlir-pdll-lsp-server/MlirPdllLspServerMain.cpp
26

This is essentially just for the tests of the server. The server does support using split file markers for PDLL files though.

41

Yep.

mlir/tools/mlir-pdll-lsp-server/mlir-pdll-lsp-server.cpp
15

Probably, but I don't think it's hugely important here given that this isn't a tool that needs to be reimplemented by users. If/when we do hoist that somewhere more usable, we could fixup this though.

mlir/utils/vscode/src/configWatcher.ts
49

I didn't change it because it would break all existing extension users, given that it changes the name of the setting used for providing the server path. Do you think we should change it (given the impact on users)? If we do, it should likely be in it's own commit to provide proper justification.

This revision was automatically updated to reflect the committed changes.
mlir/tools/mlir-pdll-lsp-server/mlir-pdll-lsp-server.cpp