-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
For loops: Allow sharing variables with main program #3014
base: master
Are you sure you want to change the base?
Conversation
915ca3b
to
f00dedd
Compare
14cc73e
to
70e0798
Compare
1. Determine which variables need to be shared with the loop callback 2. Pack pointers to them into a context struct 3. Pass pointer to the context struct to the callback function 4. In the callback, override the shared variables so that they read and write through the context pointers instead of directly from their original addresses See the comment in semantic_analyser.cpp for pseudo code of this transformation.
@@ -351,19 +352,27 @@ void Printer::visit(While &while_block) | |||
} | |||
} | |||
|
|||
void Printer::visit(For &for_loop) | |||
void Printer::visit(For &f) |
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: I know f
is more terse but I much prefer the variable name for_loop
.
@@ -176,7 +182,7 @@ class StructManager { | |||
|
|||
private: | |||
std::map<std::string, std::shared_ptr<Struct>> struct_map_; | |||
std::unordered_set<std::shared_ptr<Struct>> tuples_; | |||
std::unordered_set<std::shared_ptr<Struct>> anonymous_types_; |
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.
naming nit: maybe make this consistent with the method e.g. anon_types_
or AddAnonymousStruct
@@ -259,7 +258,11 @@ class CodegenLLVM : public Visitor { | |||
int current_usdt_location_index_{ 0 }; | |||
bool inside_subprog_ = false; | |||
|
|||
std::map<std::string, AllocaInst *> variables_; | |||
struct VariableLLVM { | |||
llvm::Value *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.
Why make the value generic, it seems like all the places where this is used are creating a AllocaInst
?
llvm::Value *value; | ||
llvm::Type *type; | ||
}; | ||
std::map<std::string, VariableLLVM> variables_; |
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.
Since this is class level is there a case of possible collision where two (or more probes) use the same scratch variable name and reference them in a loop?
Or no, because each prog has it's own ELF file...
ctx_t = b_.GetStructType("ctx_t", ctx_field_types); | ||
ctx = b_.CreateAllocaBPF(ctx_t, "ctx"); | ||
|
||
for (size_t i = 0; i < ctx_fields.size(); i++) { |
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.
super duper nit
for (size_t i = 0; i < ctx_fields.size(); i++) { | |
for (size_t i = 0; i < ctx_fields.size(); ++i) { |
auto *field_expr = variables_[field.name].value; | ||
auto *field_ty = field_expr->getType(); |
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.
super nit (makes it slightly easier to grok IMHO)
auto *field_expr = variables_[field.name].value; | |
auto *field_ty = field_expr->getType(); | |
auto *field_ty = variables_[field.name].value->getType(); |
* write through the context pointers instead of directly from their | ||
* original addresses | ||
* | ||
* Example transformation with context: |
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 like the example!
// function as the context parameter. | ||
CollectNodes<Variable> vars_referenced; | ||
std::unordered_set<std::string> var_set; | ||
for (auto *stmt : *f.stmts) { |
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: looks like you could combine these two loops to get referenced and new vars.
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.
Few small questions and nits but otherwise LGTM.
This lets programs such as this run:
Implementation Details
Pseudo code for the transformation we apply:
Before:
After:
Checklist
N/A: For-loops haven't been released yet and this is basic functionality expected of for-loops so is covered by the existing docs/changelog.
- [ ] Language changes are updated inman/adoc/bpftrace.adoc
and if needed indocs/reference_guide.md
- [ ] User-visible and non-trivial changes updated inCHANGELOG.md