-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[IR] Add an IR Builder with some basic functions #2204
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A few nits
|
||
class IRBuilder { | ||
public: | ||
struct InsertPoint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: does this need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need to modify the insertion point... I think it's fine to make it private now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a great first step!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! LGTM.
However, we cannot use the new
IRBuilder
to build a whole program now, since the constructorKernel::Kernel()
fetches the IR fromASTBuilder
. I'm not sure how to solve it -- would it be fine to simply add a constructor to theKernel
class, and let developers call it to use the newIRBuilder
?
Your solution sounds good!
Stmt *insert(std::unique_ptr<Stmt> &&stmt); | ||
|
||
// Constants. TODO: add more types | ||
Stmt *get_int32(int32 value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may also consider a template <typename T> Stmt *get_const(T val)
template function and then specialize for different types (or use if constexpr (std::is_same_v<T, int32>)
). Just a random idea and the decision is yours :-)
Related issue = #2193
This PR renames the existing
IRBuilder
class toASTBuilder
, since it actually builds the frontend AST in C++.The
IRBuilder
newly added in this PR is able to build the following instructions now:DataType dt, bool is_ptr
arguments is a good idea)However, we cannot use the new
IRBuilder
to build a whole program now, since the constructorKernel::Kernel()
fetches the IR fromASTBuilder
. I'm not sure how to solve it -- would it be fine to simply add a constructor to theKernel
class, and let developers call it to use the newIRBuilder
?And... the new
IRBuilder
is not unit-tested now.[Click here for the format server]