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] Remove v62 support #7608

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

aankit-ca
Copy link
Contributor

Remove v62 and make v65 as the default isa version for HVX.
@pranavb-ca @steven-johnson

Remove v62 and make v65 as the default isa version for HVX.
@@ -54,10 +54,6 @@ class CodeGen_Hexagon : public CodeGen_Posix {
std::vector<Type> arg_types,
int flags);

int is_hvx_v65_or_later() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aankit-ca - where is hvx_v66 needed? I dont see any use of anything that is available at v66 but not v65, but I may be mistaken.

@steven-johnson
Copy link
Contributor

I need to do some checking regarding Google's internal usage here -- not sure if we still require support for the earlier clients (in which case we would either need to do some internal patching to maintain the old codepaths, or considering deferring this change.)

@steven-johnson
Copy link
Contributor

I'm a little concerned that the approach here makes it nonobvious that the baseline is generating... previously, if you just specified hvx you'd get v60, whereas after this, the same build commands would quietly give you v65. I wonder if a safer approach might be:

  • Remove the hvx_v62 feature flag entirely
  • Keep the hvx_v65
  • For now, require that any target that has hvx set also has hvx_v65 (or later) set

This makes for a more-verbose target string, but it does ensure that the meaning of a given Halide target string doesn't silently change codegen (although it may fail to compile, e.g. if you try to specify v62, but a loud failure is much better than a quiet change to codegen)

@steven-johnson
Copy link
Contributor

After some internal consultation, it seems likely that Google will want to continue to support v60/v62 codegen in Halide for another year or so.

Can you share some detail on the motivation to remove these codegen options?

@pranavb-ca
Copy link
Contributor

After some internal consultation, it seems likely that Google will want to continue to support v60/v62 codegen in Halide for another year or so.

Can you share some detail on the motivation to remove these codegen options?

The motivation to remove this is maintenance and testing. It is easier to maintain the downstream repository as both our downstream Halide and LLVM have removed support for v62 (fairly old architecture). These are supported on release branches internally. v60 was removed from the upstream Halide repo when Pixel 1 was 3 years old (2019) indicating the end of support for that phone from Google. IIRC pixel 2 or 3 were where v62 first appeared and we reckoned that Google wouldnt have a problem removing support for v62 now.

The only problem leaving it here is we many not be able to upgrade upstream buildbots to a newer Hexagon SDK because newer SDKs with newer LLVM toolchains in them will not support v62. But, until we do that, we are fine holding off on this.

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.

4 participants