[PATCH] D14547: [llvm-profdata] Add support for weighted merge of profile data

[Justin Bogner via llvm-commits]

Xinliang David Li <davidxl at google.com> writes:
> On Tue, Nov 10, 2015 at 2:36 PM, Diego Novillo <dnovillo at google.com> wrote:
>> On Tue, Nov 10, 2015 at 3:07 PM, David Li <davidxl at google.com> wrote:
>>> ================
>>> Comment at: test/tools/llvm-profdata/weight-sample.test:42
>>> @@ +41,3 @@
>>> +3- Bad merge: foo and bar profiles with too few weight values
>>> +RUN: not llvm-profdata merge --sample --text -weights=2
>>> %p/Inputs/weight-sample-bar.proftext %p/Inputs/weight-sample-foo.proftext
>>> -o %t.out 2>&1 | FileCheck %s --check-prefix=ERROR1
>>> +ERROR1: error: Profile weight must be specified for each input file.
>>> ----------------
>>> No need for such tests if the weight is always attached to the input file.
>>>
>>
>> But then what happens if one of the files is specified without ":WEIGHT"?
>> Should we complain about it?  Should we assume MAX(other weights)?
>
> The default is 1 of course.
>
>> I'm not sure I'm completely convinced that file1:W1 file2:W2 is better
>> than --weights=W1,W2.  What if ':' is part of the filename?  I know, seems
>> like a stretch.  I'm not actually arguing against it.  I mostly find it a
>> bit odd.
>
> : is just a suggestion, you can use coma to separate it.
>
> pros:
>
> 1) the weight and file name are always 'together'. Think about a long list
> of files -- having a separate list option make it harder to associate and
> and  can not really skip the default weight of 1, e.g
> -W10,1,1,1,1,1,1,1,1,1,20
> 2) there is no need for an additional option
> 3) the implementation is simpler and requires fewer code.
>
> cons:
> I don't see any real cons -- except that you find it weird -- but why?

The biggest con is probably ambiguity. Using a comma isn't really any
better than using a colon here, both are perfectly valid in filenames.
I guess it's probably not likely that anyone has a file named foo and a
file named foo:10 and wants to use foo with a weight of 10, but the code
will certainly have to deal with that case if we're attaching the
weights to the filenames.