Skip to content

Change generator target for using optional target flags for apps/blur#5536

Open
aankit-ca wants to merge 1 commit intohalide:mainfrom
aankit-ca:issue5523
Open

Change generator target for using optional target flags for apps/blur#5536
aankit-ca wants to merge 1 commit intohalide:mainfrom
aankit-ca:issue5523

Conversation

@aankit-ca
Copy link

Without the change:

HL_TARGET=arm-64-android-hvx_128 make bin/arm-64-android/test

would have target=arm-64-android instead of arm-64-android-hvx_128

@dsharletg PTAL

Without the change:

HL_TARGET=arm-64-android-hvx_128 make bin/arm-64-android/test

would have target=arm-64-android instead of arm-64-android-hvx_128
@steven-johnson
Copy link
Contributor

I'm not sure I like this fix -- it changes the pattern we use ~everywhere else in Make.

Why can't the user just do make bin/arm-64-android-hvx_128/test?

@aankit-ca
Copy link
Author

aankit-ca commented Dec 8, 2020

This seemed like a quick fix for #5523.
For make bin/arm-64-android-hvx_128/test to work we will need:
CXX-arm-32-android-hvx_128 in apps/support/Makefile and as DIllon pointed out having every combination of optional flag is not feasible.
We can try breaking the HL_TARGET to see which CXX-* should be used in support/Makefile?

@dsharletg
Copy link
Contributor

I think this is a good change, but I think maybe we should go ahead and do it for all the apps?

The reason I think it is good is because the strategy used for selecting compilers in Makefile.inc doesn't work without it. We shouldn't need a different host compiler/linker flag for each possible combination of optional target flags, only the arch/OS.

Also, I think the current thing is simply broken. HL_TARGET=arm-64-android-hvx_128 make bin/arm-64-android/blur ignores HL_TARGET, that seems like a bug to me.

@steven-johnson
Copy link
Contributor

I'll prep a PR to make this change uniformly across the apps.

@abadams
Copy link
Member

abadams commented Dec 9, 2020

I don't think this is a good change. It's much better to distinguish between Halide targets using the make target. Messing with environment variables and having the makefile depend on them breaks make's dependency tracking when you're changing targets.

Most makefile rules should entirely ignore HL_TARGET. The only exception is generic targets like "test"

@abadams
Copy link
Member

abadams commented Dec 9, 2020

Here's how to select compilers for cross-compilation without depending on env vars:

FILTER_CXX ?= c++
FILTER_LDFLAGS ?= -lwhatever
FILTER_CXX_FLAGS ?= -I whatever

... the generator rules ...

$(BIN)/%/filter: filter.cpp $(BIN)/%/my_halide_filter.a $(BIN)/%/runtime.a
	@mkdir -p $(@D)
	$(FILTER_CXX) $(FILTER_CXXFLAGS) -I$(BIN)/$* -Wall -O3 $^ -o $@ $(FILTER_LDFLAGS) 

# Tell make to use a different compiler and flags for some targets:
bin/arm-64-android/filter: FILTER_CXX=$(ARM64_CXX)
bin/arm-64-android/filter: FILTER_LDFLAGS=-ldl -llog -DHALIDE_NO_JPEG -DHALIDE_NO_PNG -static-libstdc++

# You can use wildcards too:
bin/arm-64-android-%/filter: FILTER_CXX=$(ARM64_CXX)
bin/arm-64-android-%/filter: FILTER_LDFLAGS=-ldl -llog -DHALIDE_NO_JPEG -DHALIDE_NO_PNG -static-libstdc++ -lEGL

@steven-johnson
Copy link
Contributor

(See #5539 for a complete suite of this change.)

@alexreinking alexreinking added this to the v12.0.0 milestone Dec 11, 2020
@steven-johnson
Copy link
Contributor

At this point it seems like this change isn't likely to happen, at least not in this form.

@alexreinking alexreinking modified the milestones: v12.0.0, v13.0.0 Apr 16, 2021
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.

6 participants