Allow compile_to_multitarget() to emit object files #5183

steven-johnson · 2020-08-13T00:47:11Z

I think this fits the criteria you need. PTAL.

Fixes #5169

src/Module.cpp

steven-johnson · 2020-08-14T22:01:16Z

Review ping

abadams · 2020-08-15T00:16:42Z

Looks reasonable to me, but Alex's review is more important.

alexreinking · 2020-08-15T00:42:35Z

I suppose it's impossible at this point to make the names consistent between n=1 and n>1, but other than that, my only question is about the extra work of changing "-" to "_". I'm not sure why that's necessary.

alexreinking · 2020-08-15T00:20:13Z

src/Module.cpp

@@ -830,7 +833,7 @@ void compile_multitarget(const std::string &fn_name,
        }

        // Each sub-target has a function name that is the 'real' name plus a suffix
-        std::string suffix = "_" + replace_all(target.to_string(), "-", "_");
+        std::string suffix = "_" + replace_all(target_label, "-", "_");


Why do we change "-" to "_" here? What's wrong with just doing "-" everywhere?

Reasons likely lost to the mists of time now. But it's been that way for a while and some downstream systems are forced to rely on it (eg Bazel, in which the build system must know exact names for all files produced), so if we changed it here we'd have to change some downstream users, potentially. Not likely a big deal but also not a big deal to keep it this way. (For outputs that are embedded in a .a file it doesn't matter, but for other outputs, like .s files, it does.)

alexreinking · 2020-08-15T18:11:00Z

I think I want to get #5186 working before we land this to make sure I don't need any further changes here.

steven-johnson · 2020-08-17T16:34:26Z

I suppose it's impossible at this point to make the names consistent between n=1 and n>1

No, impossible at all -- it's just a policy decision.

We could declare that compile_to_static_library() maintains its current behavior, but that compile_to_object_files() always appends the suffix regardless of n. How useful/necessary would this be to the value of this call?

steven-johnson · 2020-08-17T19:18:25Z

OK, I have changed it so that compile_to_multitarget() for object output (but not static_library output) now uniformly adds the proper suffix. Also added missing Python wrappers for compile_to_multitarget_object_files() and beefed up the correctness test considerably.

(I did not change the underscore-vs-hyphen code at this point.)

PTAL.

alexreinking · 2020-08-17T19:55:14Z

OK, I have changed it so that compile_to_multitarget() for object output (but not static_library output) now uniformly adds the proper suffix.

Ok, I will update #5186

alexreinking · 2020-08-17T20:32:50Z

Looks like the n=1 case is breaking the Makefile?

steven-johnson · 2020-08-17T21:01:06Z

Looks like the n=1 case is breaking the Makefile?

whoops, fix pushed

abadams · 2020-08-17T21:04:16Z

Double checking: This doesn't change the filename resulting from compile_to_object, right?

steven-johnson · 2020-08-17T21:14:16Z

Double checking: This doesn't change the filename resulting from compile_to_object, right?

Should not change that.

steven-johnson · 2020-08-18T00:07:41Z

Hm, this still is a little ticklish: it doesn't change compile_to_object(), but it does change the behavior of running a Generator with -e object and a single target. Currently, we always output it as $FNAME.o; with this change, it would output $FNAME_$TARGET.o -- this breaks (e.g.) apps/linear_algebra, which makes a bunch of .o files, ars them together, and links them together. Options here would be:

Keep the new behavior and change the Makefile to use static_library output instead of object
Keep the new behavior and just fix the Makefile to append the target string to the .o files as necessary
Tweak this PR so that -e object does the same thing that it does now, and add a new option (-e object_with_target) that has the new behavior of adding the target string to the end of .o files. This would work but feels weirdly special-case-y and like a wart that we'd have to keep around forever.

I'm inclined to do the first. Anyone have strong feelings otherwise?

abadams · 2020-08-18T00:11:39Z

Yeah, that's the case I was concerned about. I guess I assumed it went through compile_to_object.

I feel strongly that we should preserve the existing behavior of -e o so as to not break peoples' builds, and so that you don't have to think about multitarget if it's not relevant to you.

Can't we just make -e o equivalent to compile_to_object for single-target builds?

steven-johnson · 2020-08-18T00:27:40Z

Can't we just make -e o equivalent to compile_to_object for single-target builds?

Sure. That's exactly what it was before the latest change I pushed (at Alex's request). Having a symmetry for the object-file outputs (so that they have a uniform naming convention regardless of whether you generate one or more-than one) is appealing. But it is a behavioral change. To be honest, I don't really have a dog in this fight (if I had my druthers I'd require us to always output .a files). @alexreinking, how much pain does the special-casing of single-object-file-outputs case in CMake?

(FYI: I updated the apps/linear_algebra Makefile to use static_library instead, which seems like a safe and good change regardless of what we decide here.)

abadams · 2020-08-18T00:29:20Z

Amusingly, people might be using -e o instead of static libraries specifically because build systems treat generated libraries as second class

abadams · 2020-08-18T00:34:17Z

I also like your option 3: Refusing to do multi-target with -e object, and instead requiring -e objects or -e object_bundle or something. Something that conveys that it's multiple things.

steven-johnson · 2020-08-18T00:35:11Z

Amusingly, people might be using -e o instead of static libraries specifically because build systems treat generated libraries as second class

If, by "amusingly", you mean "tragically", then yes. (Why ar has never grown the ability to combine libraries is mind-boggling)

alexreinking · 2020-08-18T00:47:40Z

Can't we just make -e o equivalent to compile_to_object for single-target builds?

This is fine.

@alexreinking, how much pain does the special-casing of single-object-file-outputs case in CMake?

While I do like the object_bundle (objects is too subtle) output option idea, the pain is not very great. This is what I'm doing now:

Halide/cmake/HalideGeneratorHelpers.cmake

Lines 142 to 151 in dfab2a2

    
                   list(APPEND GENERATOR_OUTPUTS object) 
        
                   list(LENGTH ARG_TARGETS len) 
        
                   if (len EQUAL 1) 
        
                       set(GENERATOR_SOURCES "${TARGET}${object_suffix}") 
        
                   else () 
        
                       set(GENERATOR_SOURCES wrapper ${ARG_TARGETS}) 
        
                       list(TRANSFORM GENERATOR_SOURCES REPLACE "-" "_") 
        
                       list(TRANSFORM GENERATOR_SOURCES PREPEND "${TARGET}_") 
        
                       list(TRANSFORM GENERATOR_SOURCES APPEND "${object_suffix}") 
        
                   endif ()

where ARG_TARGETS are the Halide generator targets, ${object_suffix} is either .a or .lib, and TARGET is the name of the CMake target being created (and also the -n parameter)

steven-johnson · 2020-08-18T00:54:29Z

Something about the object_bundle idea just rubs me the wrong way -- it's an exact synonym for object with one different corner case, and the idea that we are essentially adding a new corner case that we will need to support long-term makes my teeth hurt. (It's the option I like least of what I proposed, I really just listed it for completeness.)

I'm going to leave this PR as-is for tonight and ponder overnight. I welcome any more thoughts people have here.

alexreinking · 2020-08-18T00:55:43Z

I'm going to leave this PR as-is for tonight and ponder overnight. I welcome any more thoughts people have here.

I'm perfectly fine special casing for n=1 since I have to special case for n>1 anyway on account of the wrapper object that only gets generated in that case.

steven-johnson · 2020-08-18T17:12:00Z

I'm perfectly fine special casing for n=1 since I have to special case for n>1 anyway on account of the wrapper object that only gets generated in that case.

Hrm, an excellent point. OK, I'll do some selective reverting to go back to the previous behavior.

This reverts commit 01c15b4.

steven-johnson · 2020-08-18T18:17:09Z

OK, I have reverted the relevant bits of code, so single-target object files now produce the same (unadorned) filename that they always did. PTAL.

Re: the swapping of _ and - in the filenames, I can be persuaded to make that change if there is a consensus in favor of it, but I'm reluctant to do so otherwise, as it's arguably an API change. @abadams any thoughts?

abadams · 2020-08-18T21:10:47Z

I think the replacement only applies to the suffixes in multi-target, right? I'm not really sure on why those were replaced. I don't have a strong opinion on it. Are there file-systems where hyphens are a no-no?

alexreinking · 2020-08-18T21:18:13Z

Are there file-systems where hyphens are a no-no?

None that I'm aware of.

steven-johnson · 2020-08-18T21:29:41Z

The question isn't so much "why did we do this in the first place", and more "is there a compelling reason to change it now, and risk breaking downstream build systems that rely on the current naming"

abadams · 2020-08-18T22:04:20Z

I don't consider the names of the object files inside the static library to be part of the interface - wouldn't you have to be doing something pretty weird for that to matter?

steven-johnson · 2020-08-18T22:07:44Z

I don't consider the names of the object files inside the static library to be part of the interface - wouldn't you have to be doing something pretty weird for that to matter?

Yes, but Hyrum's Law suggests that someone somewhere is basing their entire career on this decision :-)

(I'll try keeping the hyphens and see what breaks.)

steven-johnson · 2020-08-19T16:40:13Z

OK, I think all the issues are addressed. PTAL.

alexreinking · 2020-08-20T20:37:57Z

OK, I think all the issues are addressed. PTAL.

Merging into #5186 now. Need to figure out how I broke WASM then this can land.

steven-johnson · 2020-08-20T20:51:45Z

@alexreinking you'll need to mark this as 'changed are addressed'

alexreinking · 2020-08-20T22:36:17Z

@alexreinking you'll need to mark this as 'changed are addressed'

I couldn't find that button, so I just hit 'approve' instead 🙃

Allow compile_to_multitarget() to emit object files (Issue #5169)

31c5959

steven-johnson requested a review from alexreinking August 13, 2020 00:47

Update Module.cpp

dee3d99

alexreinking changed the title ~~Allow compile_to_multitarget() to emit object files (Issue #5169)~~ Allow compile_to_multitarget() to emit object files Aug 13, 2020

abadams reviewed Aug 13, 2020

View reviewed changes

src/Module.cpp Outdated Show resolved Hide resolved

steven-johnson added 2 commits August 13, 2020 09:27

Update Module.cpp

ec9353a

Merge branch 'master' into srj-multiobj

933acab

steven-johnson mentioned this pull request Aug 13, 2020

Flesh out CPack packaging for releases. #5135

Merged

Merge branch 'master' into srj-multiobj

5abed01

abadams approved these changes Aug 15, 2020

View reviewed changes

alexreinking requested changes Aug 15, 2020

View reviewed changes

alexreinking mentioned this pull request Aug 15, 2020

Teach add_halide_library about cross compilation #5186

Merged

steven-johnson added 2 commits August 17, 2020 10:45

Merge branch 'master' into srj-multiobj

6117729

Smarten compile_to_multitarget

7138e9e

c_source should be single, not multi

1860455

Fix apps/linear_algebra

01c15b4

steven-johnson added 3 commits August 18, 2020 11:05

Revert "Fix apps/linear_algebra"

e76ec34

This reverts commit 01c15b4.

Update Module.cpp

8d9e782

Fixes

5886bff

steven-johnson added 2 commits August 18, 2020 15:17

Don't substitute _ for -

107064c

Merge branch 'master' into srj-multiobj

cbc7290

Merge branch 'master' into srj-multiobj

b2d1b5c

alexreinking approved these changes Aug 20, 2020

View reviewed changes

steven-johnson merged commit 25f8231 into master Aug 20, 2020

steven-johnson deleted the srj-multiobj branch August 20, 2020 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow compile_to_multitarget() to emit object files #5183

Allow compile_to_multitarget() to emit object files #5183

steven-johnson commented Aug 13, 2020 •

edited by alexreinking

Loading

steven-johnson commented Aug 14, 2020

abadams commented Aug 15, 2020

alexreinking commented Aug 15, 2020

alexreinking Aug 15, 2020

steven-johnson Aug 17, 2020 •

edited

Loading

alexreinking commented Aug 15, 2020

steven-johnson commented Aug 17, 2020

steven-johnson commented Aug 17, 2020

alexreinking commented Aug 17, 2020

alexreinking commented Aug 17, 2020

steven-johnson commented Aug 17, 2020

abadams commented Aug 17, 2020

steven-johnson commented Aug 17, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

abadams commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

alexreinking commented Aug 18, 2020 •

edited

Loading

steven-johnson commented Aug 18, 2020

alexreinking commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

alexreinking commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

steven-johnson commented Aug 19, 2020

alexreinking commented Aug 20, 2020

steven-johnson commented Aug 20, 2020

alexreinking commented Aug 20, 2020

Allow compile_to_multitarget() to emit object files #5183

Allow compile_to_multitarget() to emit object files #5183

Conversation

steven-johnson commented Aug 13, 2020 • edited by alexreinking Loading

steven-johnson commented Aug 14, 2020

abadams commented Aug 15, 2020

alexreinking commented Aug 15, 2020

alexreinking Aug 15, 2020

Choose a reason for hiding this comment

steven-johnson Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

alexreinking commented Aug 15, 2020

steven-johnson commented Aug 17, 2020

steven-johnson commented Aug 17, 2020

alexreinking commented Aug 17, 2020

alexreinking commented Aug 17, 2020

steven-johnson commented Aug 17, 2020

abadams commented Aug 17, 2020

steven-johnson commented Aug 17, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

abadams commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

alexreinking commented Aug 18, 2020 • edited Loading

steven-johnson commented Aug 18, 2020

alexreinking commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

alexreinking commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

abadams commented Aug 18, 2020

steven-johnson commented Aug 18, 2020

steven-johnson commented Aug 19, 2020

alexreinking commented Aug 20, 2020

steven-johnson commented Aug 20, 2020

alexreinking commented Aug 20, 2020

steven-johnson commented Aug 13, 2020 •

edited by alexreinking

Loading

steven-johnson Aug 17, 2020 •

edited

Loading

alexreinking commented Aug 18, 2020 •

edited

Loading