Issue #3925 : Remove hvx_64 #5365

pranavb-ca · 2020-10-15T21:46:23Z

Removes hvx_64
Adds a target feature hvx which defaults to Target::HVX_128

I think we should keep hvx_128 around for a little bit and before ripping it out and simply going by hvx

fixes #3925

dsharletg

Two things that affect a ton of instances:

Should we just make Target::HVX_128 == Target::HVX, so we can simplify all instances that check these to just check Target::HVX?
There are a lot of instances of vector_size = get_target()... ? 128 : 64 that should just be vector_size = 128 now?

dsharletg · 2020-10-15T23:20:20Z

apps/hexagon_benchmarks/median3x3_generator.cpp

-        if (get_target().features_any_of({Target::HVX_64, Target::HVX_128})) {
-            const int vector_size = get_target().has_feature(Target::HVX_128) ? 128 : 64;
+        if (get_target().features_any_of({Target::HVX, Target::HVX_128})) {
+            const int vector_size = get_target().features_any_of({Target::HVX, Target::HVX_128}) ? 128 : 64;


constant 128

pranavb-ca · 2020-10-15T23:52:08Z

Two things that affect a ton of instances:

Should we just make Target::HVX_128 == Target::HVX, so we can simplify all instances that check these to just check Target::HVX?

There are a lot of instances of vector_size = get_target()... ? 128 : 64 that should just be vector_size = 128 now?

dang it - i thought I caught all of those instances. I grepped for HVX_64 and that's why I think I missed out some of those. Will do.

pranavb-ca · 2020-10-16T00:02:04Z

Two things that affect a ton of instances:

Should we just make Target::HVX_128 == Target::HVX, so we can simplify all instances that check these to just check Target::HVX?

Actually that is what I have done, but in a sed frenzy I went ahead and changed has_feature to features_any_of

…64 now that HVX_128 is the only mode for HVX

src/Target.h

dsharletg · 2020-10-16T00:14:25Z

apps/camera_pipe/camera_pipe_generator.cpp

-        if (get_target().has_feature(Target::HVX_128)) {
-            strip_size = processed.dim(1).extent() / 2;
-        } else if (get_target().has_feature(Target::HVX_64)) {
+        if (get_target().has_feature(Target::HVX)) {
            strip_size = processed.dim(1).extent() / 4;


Keep the HVX_128 branch, not the HVX_64 branch.

I jumped the gun a little bit here. Newer cores have 4 contexts, but presently the lowest we support still has 2 contexts. I'll switch it back to / 2 ;

Oh, I see. Actually, I think having more strips than cores is fine, so maybe keeping this 4 is best.

dsharletg · 2020-10-16T00:15:29Z

src/CodeGen_Hexagon.cpp

-        {Halide::Target::HVX_128, Halide::Target::HVX_64}))
-        << "Cannot set both HVX_64 and HVX_128 at the same time.\n";
+    user_assert(target.has_feature(Target::HVX))
+        << "Creating a Codegen target for Hexagon without the hvx_128 or the hvx target feature.\n";


Remove hvx_128 from this message?

I left it in the message to indicate to users that hvx_128 is still ok to use as a feature.

dsharletg · 2020-10-16T00:17:21Z

src/Target.cpp

@@ -871,13 +870,10 @@ int Target::natural_vector_size(const Halide::Type &t) const {

    if (arch == Target::Hexagon) {
        if (is_integer) {
-            // HVX is either 64 or 128 *byte* vector size.
-            if (has_feature(Halide::Target::HVX_128)) {
+            if (features_any_of({Halide::Target::HVX, Halide::Target::HVX_128})) {


features(...)

dsharletg · 2020-10-16T00:17:32Z

src/Target.cpp

@@ -966,7 +962,7 @@ bool Target::get_runtime_compatible_target(const Target &other, Target &result)
    }

    if ((features & matching_mask) != (other.features & matching_mask)) {
-        Internal::debug(1) << "runtime targets must agree on SoftFloatABI, Debug, TSAN, ASAN, MSAN, HVX_64, HVX_128, HexagonDma, and HVX_shared_object\n"
+        Internal::debug(1) << "runtime targets must agree on SoftFloatABI, Debug, TSAN, ASAN, MSAN, HVX_128, HexagonDma, and HVX_shared_object\n"


HVX_128 -> HVX

src/Target.cpp

test/correctness/simd_op_check_hvx.cpp

dsharletg

I don't know why I can't reply to any of the existing threads on the "conversation" tab... but regarding:

I left it in the message to indicate to users that hvx_128 is still ok to use as a feature.

If it's going to be deprecated, wouldn't it make sense to just encourage use of hvx?

src/HexagonOffload.cpp

apps/camera_pipe/camera_pipe_generator.cpp

pranavb-ca · 2020-10-19T21:29:28Z

@dsharletg - Addressed comments from the last review

dsharletg

Thanks for the fixes!

Merge this after halide/Halide#5365 is merged.

steven-johnson · 2020-10-20T16:16:56Z

LGTM -- pushed a partial change to the buildbot to avoid the hvx64 build errors, then synced this to master. Let's let it go green before landing.

dsharletg · 2020-10-20T18:24:33Z

This is failing on some of the buildbots:

make: *** No rule to make target 'bin/build/initmod.hvx_64_ll.o', needed by 'bin/libHalide.so'.  Stop.
make: *** Waiting for unfinished jobs....

Looking at the PR, I don't see any changes to either Makefile or CMakeLists.txt, both should probably change.

pranavb-ca · 2020-10-20T19:36:38Z

halide/build_bot#108

dsharletg · 2020-10-20T19:39:38Z

I think that's a different thing. Yes, the hvx_64 tests will fail to run. But here, libHalide.so is failing to link at all. I think because these two lines need to be removed:

https://github.com/halide/Halide/blob/master/Makefile#L829
https://github.com/halide/Halide/blob/master/src/runtime/CMakeLists.txt#L105

pranavb-ca · 2020-10-20T20:10:38Z

I think that's a different thing. Yes, the hvx_64 tests will fail to run. But here, libHalide.so is failing to link at all. I think because these two lines need to be removed:

https://github.com/halide/Halide/blob/master/Makefile#L829
https://github.com/halide/Halide/blob/master/src/runtime/CMakeLists.txt#L105

Yes, you are right. I was simply reminded of my PR from yesterday because of your comment and so pasted that here. I had taken care of CMakeLists.txt in a previous commit. Just pushed one more change to remove it from Halide/Makefile

alexreinking · 2020-10-20T21:51:26Z

I think we should keep hvx_128 around for a little bit and before ripping it out and simply going by hvx

If we hold off merging this until v11.0.0 is released, then we don't need to wait at all.

dsharletg · 2020-10-20T23:50:44Z

This has passed all the build bots, @alexreinking is there really a reason to wait to merge this? It isn't a breaking change (unless you are using hvx_64 of course). hvx_128 remains as a useful flag, it's just deprecated (and it is only silently deprecated at this point). I think the steps here are:

Remove hvx_64 (this PR)
Deprecate hvx_128 by warning people to use hvx instead.
Remove hvx_128 and only support hvx.

I think we should do 1 now that this PR is ready to merge, and avoid generating inevitable merge conflicts for this PR to manage later.

2 and 3 aren't planned for any particular time yet. 2 and 3 should probably be in separate releases of course.

alexreinking · 2020-10-21T05:31:03Z

It's totally fine to put this through a deprecation plan. I thought this PR might not be ready for the v11.0 release, but it looks like I misread the room :)

dsharletg · 2020-10-21T05:32:36Z

OK, I think I'll let @pranavb-ca decide when to merge it. I was also mistaken, some of the build bots are still running.

steven-johnson · 2020-10-21T16:30:45Z

I agree with @dsharlet -- happy to go ahead and land before branching the 11 release. Buildbots are clean, there's just one merge conflict to resolve.

steven-johnson · 2020-10-21T16:34:40Z

UPDATE: the merge conflict was due to my clang-tidy changes, so I went ahead and pushed the fix. Should be good to merge anytime.

steven-johnson · 2020-10-21T20:20:27Z

Green, OK to land

pranavb-ca added 12 commits October 14, 2020 17:15

Remove MAKE_ID_PAIR and IdPair

048999c

remove the uses of hvx_64 from Halide/src

6371ef9

Remove use of hvx_64 from Halide/test

b121015

Remove use of hvx_64 from apps

18b704b

remove use of hvx_64 from Target.cpp

a33b3fc

fix intrinsic ids

2554e60

Remove all definitions of hvx_64

3316566

Look for Target::HVX too, everywhere that we look for Target::HVX_128

609840c

Clean up some nonsensical code related to hvx in apps

9a96a46

[camera_pipe] - In hvx_128 we need 4 threads to saturate hvx with work

09f9eda

Merge branch 'master' into pdb_remove_hvx_v64

13a4eb1

Fix bad merge of test/correctness/mul_div_mod.cpp

4d1a4bb

pranavb-ca requested review from steven-johnson and dsharletg October 15, 2020 21:46

Remove hvx_64 and hvx to python bindings

04cd8dd

dsharletg requested changes Oct 15, 2020

View reviewed changes

pranavb-ca added 2 commits October 15, 2020 19:04

Check only for Target::HVX

8151b77

Set vector_size to 128. rule out vector sizes that made sense on HVX_…

16604ae

…64 now that HVX_128 is the only mode for HVX

pranavb-ca commented Oct 16, 2020

View reviewed changes

src/Target.h Outdated Show resolved Hide resolved

dsharletg reviewed Oct 16, 2020

View reviewed changes

prefer using HVX over HVX_128

2cde234

dsharletg reviewed Oct 16, 2020

View reviewed changes

src/HexagonOffload.cpp Show resolved Hide resolved

apps/camera_pipe/camera_pipe_generator.cpp Outdated Show resolved Hide resolved

alexreinking added this to the v12.0.0 milestone Oct 16, 2020

Address review comments

85f143c

dsharletg approved these changes Oct 19, 2020

View reviewed changes

pranavb-ca added a commit to pranavb-ca/build_bot that referenced this pull request Oct 20, 2020

Remove the testing of hvx_64 and rename hvx_128 to hvx.

b5242e3

Merge this after halide/Halide#5365 is merged.

pranavb-ca mentioned this pull request Oct 20, 2020

Remove the testing of hvx_64 and rename hvx_128 to hvx. halide/build_bot#108

Merged

steven-johnson approved these changes Oct 20, 2020

View reviewed changes

Merge branch 'master' into pdb_remove_hvx_v64

00ae979

steven-johnson added 2 commits October 20, 2020 09:26

clang-format

7fdd42c

clang-format

5e91d6f

remove hvx_64 from Halide/Makefile

c2c35b3

Merge branch 'master' into pdb_remove_hvx_v64

b2c9769

Merge branch 'master' into pdb_remove_hvx_v64

e520503

Update CodeGen_Hexagon.cpp

d94e7a7

alexreinking modified the milestones: v12.0.0, v11.0.0 Oct 21, 2020

pranavb-ca merged commit 31f1937 into master Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3925 : Remove hvx_64 #5365

Issue #3925 : Remove hvx_64 #5365

pranavb-ca commented Oct 15, 2020

dsharletg left a comment

dsharletg Oct 15, 2020

pranavb-ca commented Oct 15, 2020

pranavb-ca commented Oct 16, 2020

dsharletg Oct 16, 2020

pranavb-ca Oct 16, 2020

dsharletg Oct 16, 2020

dsharletg Oct 16, 2020

pranavb-ca Oct 16, 2020

dsharletg Oct 16, 2020

dsharletg Oct 16, 2020

dsharletg left a comment

pranavb-ca commented Oct 19, 2020

dsharletg left a comment

steven-johnson commented Oct 20, 2020

dsharletg commented Oct 20, 2020

pranavb-ca commented Oct 20, 2020

dsharletg commented Oct 20, 2020

pranavb-ca commented Oct 20, 2020

alexreinking commented Oct 20, 2020

dsharletg commented Oct 20, 2020

alexreinking commented Oct 21, 2020

dsharletg commented Oct 21, 2020

steven-johnson commented Oct 21, 2020

steven-johnson commented Oct 21, 2020

steven-johnson commented Oct 21, 2020

Issue #3925 : Remove hvx_64 #5365

Issue #3925 : Remove hvx_64 #5365

Conversation

pranavb-ca commented Oct 15, 2020

dsharletg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavb-ca commented Oct 15, 2020

pranavb-ca commented Oct 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsharletg left a comment

Choose a reason for hiding this comment

pranavb-ca commented Oct 19, 2020

dsharletg left a comment

Choose a reason for hiding this comment

steven-johnson commented Oct 20, 2020

dsharletg commented Oct 20, 2020

pranavb-ca commented Oct 20, 2020

dsharletg commented Oct 20, 2020

pranavb-ca commented Oct 20, 2020

alexreinking commented Oct 20, 2020

dsharletg commented Oct 20, 2020

alexreinking commented Oct 21, 2020

dsharletg commented Oct 21, 2020

steven-johnson commented Oct 21, 2020

steven-johnson commented Oct 21, 2020

steven-johnson commented Oct 21, 2020