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

[Ansor][AutoTVM v2.0] Phase 1: feature extraction for cost models #6190

Merged
merged 10 commits into from
Aug 12, 2020

Conversation

merrymercy
Copy link
Member

@merrymercy merrymercy commented Aug 2, 2020

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

  1. Uses a general format for all loop structures, so all workloads can share the same cost model.
  2. Includes more analysis results (e.g., reuse distance) into the feature vector.

@merrymercy merrymercy changed the title [Ansor][AutoTVM v2.0] Phase 1 feature extraction for xgboost model [Ansor][AutoTVM v2.0] Phase 1: feature extraction for cost models Aug 2, 2020
@merrymercy
Copy link
Member Author

cc @tqchen @junrushao1994 @jcf94 @comaniac @whbldhwj

@merrymercy merrymercy force-pushed the pr-feature-extraction branch 5 times, most recently from 0ace7c7 to 710ef76 Compare August 3, 2020 21:45
Copy link
Contributor

@comaniac comaniac left a 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.

include/tvm/auto_scheduler/feature.h Outdated Show resolved Hide resolved
include/tvm/auto_scheduler/feature.h Outdated Show resolved Hide resolved
include/tvm/auto_scheduler/feature.h Outdated Show resolved Hide resolved
include/tvm/auto_scheduler/feature.h Outdated Show resolved Hide resolved
python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
src/auto_scheduler/feature.cc Outdated Show resolved Hide resolved
src/auto_scheduler/feature.cc Outdated Show resolved Hide resolved
src/auto_scheduler/feature.cc Outdated Show resolved Hide resolved
@merrymercy merrymercy force-pushed the pr-feature-extraction branch from 55c4538 to 9105a96 Compare August 4, 2020 14:31
Copy link
Contributor

@comaniac comaniac left a 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?

python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
@merrymercy
Copy link
Member Author

@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.

src/auto_scheduler/feature.cc Outdated Show resolved Hide resolved
python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
# The format for n records is:
# {
# int n;
# int[n+2] sizes
Copy link
Member

Choose a reason for hiding this comment

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

nitpick on the doc

Suggested change
# int[n+2] sizes
# int sizes[0]
# ...
# int sizes[n + 1]

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]]".

Copy link
Member Author

@merrymercy merrymercy Aug 12, 2020

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)
Copy link
Member

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?

Copy link
Member Author

@merrymercy merrymercy Aug 11, 2020

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

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.

python/tvm/auto_scheduler/feature.py Outdated Show resolved Hide resolved
src/auto_scheduler/feature.cc Outdated Show resolved Hide resolved
src/auto_scheduler/feature.cc Outdated Show resolved Hide resolved
src/auto_scheduler/feature.cc Show resolved Hide resolved
@tqchen tqchen self-assigned this Aug 10, 2020
@tqchen tqchen added status: need review status: need update need update based on feedbacks labels Aug 10, 2020
@tqchen
Copy link
Member

tqchen commented Aug 10, 2020

per discussion with @merrymercy :

  • split the feature extraction logic into FeatureExtractor(callback by visitor) and visitor.
  • Provide FeatureExtractor for each group of features

@merrymercy merrymercy force-pushed the pr-feature-extraction branch from 9105a96 to 782c542 Compare August 11, 2020 06:30
@merrymercy
Copy link
Member Author

merrymercy commented Aug 11, 2020

I organized the features into 5 groups.

  // Group 1: Computation related features
  // Group 2: Buffer access related features (per buffer)
  // Group 3: Arithmetic intensity related features
  // Group 4: Allocation related features
  // Group 5: Outer scope related features

The specification can be found in src/auto_scheduler/feature.cc::FeatureSet.

Each group has one corresponding extraction function, they are called in the main visitor (PerStoreFeatureExtractor).

  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.
I don't like registration or adding an extra layer of callback. It is over design and make things more complicated.

@tqchen @comaniac @FrozenGene @jroesch @junrushao1994 Your comments are all addressed. This PR is ready to be merged.

Copy link
Contributor

@comaniac comaniac left a 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

@tqchen
Copy link
Member

tqchen commented Aug 11, 2020

@merrymercy please fix the conflict

Copy link

@yangjunpro yangjunpro left a 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.

include/tvm/auto_scheduler/feature.h Show resolved Hide resolved
# The format for n records is:
# {
# int n;
# int[n+2] sizes

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)

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.

@merrymercy merrymercy requested a review from tqchen August 12, 2020 11:23
@merrymercy merrymercy merged commit 3565889 into apache:master Aug 12, 2020
@tqchen tqchen added status: accepted and removed status: need review status: need update need update based on feedbacks labels Aug 12, 2020
wjliu1998 pushed a commit to wjliu1998/incubator-tvm that referenced this pull request Aug 13, 2020
…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
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
…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
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
…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
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
…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
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
…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
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
…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
@merrymercy merrymercy deleted the pr-feature-extraction branch September 27, 2020 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants