[PATCH] D18213: Add a module Hash in the bitcode and the combined index.

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Mar-22, at 16:25, Mehdi AMINI <mehdi.amini at apple.com> wrote:
> 
> joker.eph added a comment.
> 
> Thanks for the review Teresa, I updated accordingly (but for the FileCheck, see comment below).
> 
> 
> ================
> Comment at: test/Bitcode/module_hash.ll:6
> @@ +5,3 @@
> +; RUN: llvm-bcanalyzer -dump %t | FileCheck %s --check-prefix=MOD1
> +; MOD1: <HASH op0=4294068846 op1=628906511 op2=4294967218 op3=4294967275 op4=4294967289/>
> +; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=MOD2
> ----------------
> tejohnson wrote:
>> Won't these values change frequently as the compiler changes and generates slightly different bitcode? How about just checking if the per module values match the combined index values via FileCheck variables/pattern matching?
> The hope is that a "void foo()" won't change much, but yeah it's absolutely not nice.
> What you suggest is fine for testing that we correctly read and propagate to the combined index. 
> HoweverI'm not comfortable not checking a value, because we lose coverage for the production of the hash. I.e. writing only zero in the first place would make the test pass :(
> Would it be enough to add a comment here saying that is fine updating the value is someone change the IR/bitcode?
> I'm gonna CC Duncan for an opinion on this.

Sorry I didn't respond earlier.

I agree with Teresa that we shouldn't be checking in brittle hash values
here.  It sets a bad precedent for testcase maintainability.  If you can't
get FileCheck variables and patterns to do what you want (and can't
augment FileCheck to get there), then I don't think this is the right
place for the test.

Maybe you don't need to test the value of the hash itself, but rather that
it changes when the interesting content changes.  You might be able to do
this more easily somewhere in unittests/Bitcode/ with tiny inputs.

> Index: test/Bitcode/module_hash.ll
> ===================================================================
> --- /dev/null
> +++ test/Bitcode/module_hash.ll
> @@ -0,0 +1,27 @@
> +; Check per module hash
> +; RUN: llvm-as  -module-hash  %s -o - | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=MOD1
> +; MOD1: <HASH op0=4294955122 op1=4288438281 op2=4294964550 op3=3392168298 op4=4294967184/>
> +; RUN: llvm-as  -module-hash  %p/Inputs/module_hash.ll -o - | llvm-bcanalyzer -dump | FileCheck %s --check-prefix=MOD2
> +; MOD2: <HASH op0=4294952553 op1=4294967242 op2=4294963208 op3=4294967277 op4=4294967193/>
> +
> +; Check that the hash matches in the combined index

Period at the end of the sentence.

> +
> +; First check the module themselve, note that the hash differs from above because the summary was not written previously

Please reflow this line.  Period at end of sentence.

> +; RUN: llvm-as  -module-hash -module-summary %s -o %t
> +; RUN: llvm-bcanalyzer -dump %t | FileCheck %s --check-prefix=MOD_WITH_SUMMARY1
> +; MOD_WITH_SUMMARY1: <HASH op0=4294967222 op1=4294940488 op2=3845607783 op3=4294942267 op4=4294967256/>
> +; RUN: llvm-as  -module-hash -module-summary %p/Inputs/module_hash.ll -o %t2
> +; RUN: llvm-bcanalyzer -dump %t2 | FileCheck %s --check-prefix=MOD_WITH_SUMMARY2
> +; MOD_WITH_SUMMARY2: HASH op0=4294967236 op1=209283912 op2=4294936372 op3=4293197927 op4=4294967267/>
> +
> +; Check that the hashes are propagated in the combined index (should match the one just above)

Please reflow this line.  Period at end of sentence.

> +; RUN: llvm-lto --thinlto-action=thinlink -o %t3 %t %t2
> +; RUN: llvm-bcanalyzer -dump %t3 | FileCheck %s --check-prefix=COMBINED
> +; COMBINED-DAG: <HASH abbrevid=7 op0=4294967222 op1=4294940488 op2=3845607783 op3=4294942267 op4=4294967256/>
> +; COMBINED-DAG: <HASH abbrevid=7 op0=4294967236 op1=209283912 op2=4294936372 op3=4293197927 op4=4294967267/>
> +
> +
> +; Needs a function for the combined index to be populated

Period at the end of the sentence.

> +define void @foo() {
> +    ret void
> +}
> Index: test/Bitcode/Inputs/module_hash.ll
> ===================================================================
> --- /dev/null
> +++ test/Bitcode/Inputs/module_hash.ll
> @@ -0,0 +1,4 @@
> +; Needs a function for the combined index to be populated

Period at the end of the sentence.

> +define void @bar() {
> +    ret void
> +}