Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to start interactive server for Parser dialect conversion #768

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

frabert
Copy link
Contributor

@frabert frabert commented Jan 27, 2025

Known issues:

  • The conversion pass uses a shared_ptr to the server in order to allow the pass to be cloneable -- it's not super elegant
  • This currently asks the user for a lot of information about functions

@frabert frabert force-pushed the frabert/server branch 5 times, most recently from 63106ed to 3d2f75d Compare January 27, 2025 13:43
@frabert frabert changed the title Frabert/server Add option to start interactive server for Parser dialect conversion Jan 27, 2025
@frabert frabert marked this pull request as ready for review January 27, 2025 14:10
@frabert frabert requested a review from xlauko as a code owner January 27, 2025 14:10
virtual size_t write_some(std::span< const char > dst) = 0;

// Upon completion, `dst` is filled with data.
void read_all(std::span< char > dst) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work as dst is not output parameter and you overwrite it.

You are only lucky it is used with single character span.

Also why to introduce unnecessary complexity when only thing you use is read_byte? read_some is also nowhere else used then in read of single byte.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the thing about dst being overwritten

re read_some: read_some is the only semantic that most APIs offer, but for convenience sake I also want read_all semantics, for example when reading a request's body.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you expect to be in dst at the end of read_all? You rewrite it at every iteration what is invisible from outside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dst is a span -- it's essentially a pointer char*. It's equivalent to

void read_all(char *start, char *end) {
  while(start != end) {
    do_something(start);
    ++start;
  }
}

dst.subspan returns a new span that points to the same data but nread characters ahead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I misunderstood it. I guess it works :)

}

function_model
ask_user_for_function_model(vast::server::server_base &server, hl::FuncOp op) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use core::function_op_interface everywhere instead of hl::FuncOp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find an equivalent of getSymName() for FunctionOpInterface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dyn_cast<SymbolOpInterface>(op.getOperation()).getName()

Copy link
Member

@xlauko xlauko Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and some checks you really have symbol

Copy link
Contributor

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v19.1.1) reports: 1 concern(s)
  • include/vast/Conversion/Parser/Passes.hpp:22:14: error: [clang-diagnostic-error]

    'vast/Conversion/Parser/Passes.h.inc' file not found

       22 |     #include "vast/Conversion/Parser/Passes.h.inc"
          |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Have any feedback or feature suggestions? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants