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

[Mehdi Amini via llvm-commits]

> On Mar 27, 2016, at 4:52 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> 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.

I thought about adding an optional check in the bitcodereader, so that I could add a check for something like "llvm-dis --check-module-hash" to ensure that the hash is valid.
Thoughts?

-- 
Mehdi



> 
> 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
>> +}