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

[Hexagon] Do not use target test fixture in Hexagon tests #12981

Merged
merged 9 commits into from
Oct 5, 2022

Conversation

mehrdadh
Copy link
Member

@mehrdadh mehrdadh commented Oct 4, 2022

target is test fixture for TVM CI which includes multiple targets. However, for hexagon we specify the target in each test. Adding target in the test function would result in running those tests multiple times in the CI.

This PR also refactors hexagon target in a function

cc @cconvey @csullivan @Lunderberg

Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

All the individual changes look good, just a question on the design.

  1. Should the tvm.target.hexagon function also set the host? That
    would remove the need for a separate testing-specific
    get_hexagon_target function.

  2. Should we have a separate hexagon_target fixture in
    python/tvm/contrib/hexagon/pytest_plugin.py, rather than a
    utility function?

    hexagon_cpu_version = tvm.testing.parameter('v69')
    
    @tvm.testing.fixture
    def hexagon_target(hexagon_cpu_version):
        return tvm.target.hexagon(hexagon_cpu_version)

    This would also make it easier to run tests with multiple different
    cpu versions, or to restrict a test to a single version by using
    @pytest.mark.parametrize('hexagon_cpu_version', ['v68']).

):
"""conv2d-conv2d test"""
# TODO: no support for padding in conv2d #2
pad2 = 0

target = "llvm"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like none of the tests in this file actually exercise Hexagon, and only validate that the schedules used elsewhere have the expected behavior. Are they intended to exercise Hexagon hardware at some point? If not, we should keep the same target fixture as non-Hexagon targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I reverted it. I did since I don't like the noise in the skip tests but we should fix that in TVM generally instead of hard coding the target here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I go back and forth on that part. I tend to prefer the explicitly reported and skipped tests, because otherwise a skipped test can appear as a successful test. But I also agree that having lists upon lists of skipped tests isn't useful for an interactive environment, and just adds noise.

)
tune_tasks([task], **options)


if __name__ == "__main__":
sys.exit(pytest.main(sys.argv))
tvm.testing.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

Side comment, thank you for updating these calls to use tvm.testing.main(). I'd like to make a custom lint rule for it at some point, but there isn't one at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah that would be great! I mentioned it to OSS team before.

@mehrdadh
Copy link
Member Author

mehrdadh commented Oct 5, 2022

@Lunderberg thanks for the review!
Regarding your suggestions:

  1. We still need a separate host target for AOT mode. Therefore we cannot combine the host target creation in the target fixture.
  2. I very much like this, but I suggest we do it once we have consolidated the hexagon target and made a decision about why we have hexagon target in addition to CPU target. Because that that point we have a clear definition for hexagon target for both AOT and Graph. wdyt?

@Lunderberg
Copy link
Contributor

  1. Sounds good, and thank you for pointing out the c vs llvm host. Thinking through it again, it also wouldn't be implementable using the set_target_parser in target_kind.cc, so it's better to keep them separate.

  2. That sounds reasonable, since we do have that difference for the host.

@mehrdadh mehrdadh merged commit 2e257f0 into apache:main Oct 5, 2022
@mehrdadh mehrdadh deleted the hexagon/test_refactor_target branch October 5, 2022 20:18
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
…2981)

* remove target from test functions

* refactor target_hexagon

* refactor target

* fix permission

* cleanup

* fix target

* remove target fixture from test_2d_physical_buffers

* fix target fixture in test_hexagon/conv2d tests

* address comments
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