[PATCH] D15306: [llvm-profdata] Add support for weighted merge of profile data (2nd try)

[Sean Silva via llvm-commits]

silvas added a comment.

Some nits but otherwise this LGTM.


================
Comment at: docs/CommandGuide/llvm-profdata.rst:39
@@ -38,3 +38,3 @@
 indexed profile data file.
 
 OPTIONS
----------------
Can you add a description of the weight stuff here and show the 3 example invocations I mentioned in http://reviews.llvm.org/D15306#307727 ?

================
Comment at: test/tools/llvm-profdata/weight-instr.test:6
@@ +5,3 @@
+RUN: llvm-profdata show --instr -all-functions %t | FileCheck %s --check-prefix=WEIGHT1
+RUN: llvm-profdata merge --instr --weighted-input=1,%p/Inputs/weight-instr-bar.profdata %p/Inputs/weight-instr-foo.profdata -o %t
+RUN: llvm-profdata show --instr -all-functions %t | FileCheck %s --check-prefix=WEIGHT1
----------------
The documentation says `-weighted-input` do we accept double dash or single dash or both? Let's make sure our tests are using the same command line invocations as our documentation indicates.

================
Comment at: test/tools/llvm-profdata/weight-instr.test:9
@@ +8,3 @@
+WEIGHT1: Counters:
+WEIGHT1:   usage:
+WEIGHT1:     Hash: 0x0000000000000000
----------------
Just for safety's sake, can you use `CHECK-NEXT` on the subsequent lines?

================
Comment at: test/tools/llvm-profdata/weight-instr.test:20
@@ +19,3 @@
+WEIGHT1:     Counters: 3
+WEIGHT1:     Function count: 866988873
+WEIGHT1:   main:
----------------
Is there a more useful name than `WEIGHT1`? Maybe `WEIGHT_UNIT_SCALING` or something?

================
Comment at: test/tools/llvm-profdata/weight-instr.test:33
@@ +32,3 @@
+RUN: llvm-profdata show --instr -all-functions %t | FileCheck %s --check-prefix=WEIGHT2
+WEIGHT2: Counters:
+WEIGHT2:   usage:
----------------
Can you use a more meaningful name than `WEIGHT2` Maybe `WEIGHT_3x_5x`?

================
Comment at: test/tools/llvm-profdata/weight-instr.test:57
@@ +56,3 @@
+RUN: not llvm-profdata merge --instr --weighted-input=3,%p/Inputs/weight-instr-bar.profdata --weighted-input=0,%p/Inputs/weight-instr-foo.profdata -o %t.out 2>&1 | FileCheck %s --check-prefix=BADWEIGHT1
+RUN: not llvm-profdata merge --instr --weighted-input=3,%p/Inputs/weight-instr-bar.profdata --weighted-input=0.75,%p/Inputs/weight-instr-foo.profdata -o %t.out 2>&1 | FileCheck %s --check-prefix=BADWEIGHT1
+RUN: not llvm-profdata merge --instr --weighted-input=3,%p/Inputs/weight-instr-bar.profdata --weighted-input=-5,%p/Inputs/weight-instr-foo.profdata -o %t.out 2>&1 | FileCheck %s --check-prefix=BADWEIGHT1
----------------
Can you come up with some semantically meaningful naming for the CHECK lines? Here and elsewhere.

================
Comment at: test/tools/llvm-profdata/weight-sample.test:56
@@ +55,2 @@
+RUN: not llvm-profdata merge --sample -o %t.out 2>&1 | FileCheck %s --check-prefix=NOINPUTS1
+NOINPUTS1: error: No input files specified. See llvm-profdata{{(\..+)?}} merge -help
----------------
These types of tests (that depend on system error messages or file paths) are notoriously brittle. I suggest searching around and looking at the patterns that other tests are using to know which regexes to use.

For example:
```
tools/llvm-size/basic.test:ENOENT: {{.*}}llvm-size{{(\.EXE|\.exe)?}}: {{.*}}.blah: {{[Nn]}}o such file or directory
```


http://reviews.llvm.org/D15306