-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Ansor][AutoTVM v2.0] Phase 1: feature extraction for cost models #6190
Conversation
0ace7c7
to
710ef76
Compare
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.
Reviewed all but feature.cc. Will finish it by tomorrow.
55c4538
to
9105a96
Compare
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.
Tried to review the features but they are too specific. Do you think if we can make up a registration mechanism like register("feature_name", extractor)
to better manage them?
@comaniac It is not easy to do so. Now we share a lot of computation for different features. If we compute them separately, redundant computation will be introduced. In addition, the global context is also shared for these features. How to register the required global context for a specific feature is also a problem. |
# The format for n records is: | ||
# { | ||
# int n; | ||
# int[n+2] sizes |
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.
nitpick on the doc
# int[n+2] sizes | |
# int sizes[0] | |
# ... | |
# int sizes[n + 1] |
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.
Personally, I think junrushao1994's suggestion is clearer than current comment convention.Since I think "float sizes[n + 1]" illustrates the format semantics more precisely than "float[sizes[0]]".
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 won't take either of your suggestions. I use int[x] variable
to denote an array of x integer values with the name variable
.
@junrushao1994 's suggestion is wrong because int size[n]
has another meaning of n+1 integers, but here we actually mean nth integer.
@yangjunpro 's suggestion is also wrong. It makes sizes
become the name of the filed, but actually size
only specifies the size.
n_stmts = struct.unpack_from("f", byte_arr, offset=offset) | ||
offset += SIZE_OF_FLOAT | ||
|
||
n_stmts = int(n_stmts[0] + 0.5) |
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.
perhaps add a comment here, like "for avoiding rounding error"? btw, why we use a float for integer?
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.
Some of them are int while the others are float. I want to store all of them in a single array, but we do not have union in tvm::Object. So I use a single float array to store both int and float
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.
+1, how about store the float array and the n_stmts separately? By doing this we may need to add extra code, but I think the program semantic is clearer.
per discussion with @merrymercy :
|
9105a96
to
782c542
Compare
I organized the features into 5 groups.
The specification can be found in Each group has one corresponding extraction function, they are called in the main visitor ( void VisitStmt_(const BufferStoreNode* node) final {
...
// Group 1: Computation related features
ExtractComputationFeature(node, math_op_counter);
// Group 2: Buffer access related features (per buffer)
ExtractBufferAccessFeature(node, math_op_counter, &cur_compute_ops, &compute_ops_list,
&mem_bytes_list);
// Group 3: Arithmetic intensity related features
ExtractArithmeticIntensityFeature(node, cur_compute_ops, compute_ops_list, mem_bytes_list);
// Group 4: Allocation related features
ExtractOuterScopeFeature(node);
}
void VisitStmt_(const BufferRealizeNode* node) final {
StmtExprVisitor::VisitStmt_(node);
// Group 5: Outer scope related features
ExtractAllocationFeature(node);
} I think the code is very clean and much better than the old autotvm now. @tqchen @comaniac @FrozenGene @jroesch @junrushao1994 Your comments are all addressed. This PR is ready to be merged. |
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.
Much cleaner now. LGTM
@merrymercy please fix the conflict |
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.
Just some nitpicking comments.
# The format for n records is: | ||
# { | ||
# int n; | ||
# int[n+2] sizes |
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.
Personally, I think junrushao1994's suggestion is clearer than current comment convention.Since I think "float sizes[n + 1]" illustrates the format semantics more precisely than "float[sizes[0]]".
n_stmts = struct.unpack_from("f", byte_arr, offset=offset) | ||
offset += SIZE_OF_FLOAT | ||
|
||
n_stmts = int(n_stmts[0] + 0.5) |
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.
+1, how about store the float array and the n_stmts separately? By doing this we may need to add extra code, but I think the program semantic is clearer.
…ache#6190) * [AutoScheduler] add feature extraction * fix lint * fix gpu test * address comments * improve flop estimation * rebase * refactor with group * fix * Apply suggestions from code review
…ache#6190) * [AutoScheduler] add feature extraction * fix lint * fix gpu test * address comments * improve flop estimation * rebase * refactor with group * fix * Apply suggestions from code review
…ache#6190) * [AutoScheduler] add feature extraction * fix lint * fix gpu test * address comments * improve flop estimation * rebase * refactor with group * fix * Apply suggestions from code review
…ache#6190) * [AutoScheduler] add feature extraction * fix lint * fix gpu test * address comments * improve flop estimation * rebase * refactor with group * fix * Apply suggestions from code review
…ache#6190) * [AutoScheduler] add feature extraction * fix lint * fix gpu test * address comments * improve flop estimation * rebase * refactor with group * fix * Apply suggestions from code review
…ache#6190) * [AutoScheduler] add feature extraction * fix lint * fix gpu test * address comments * improve flop estimation * rebase * refactor with group * fix * Apply suggestions from code review
For the full upstream plan, see Ansor RFC.
This PR adds feature extraction for the cost models.
It is similar to the existing feature extraction in autotvm but