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

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

> On 2016-Mar-28, at 16:43, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
>> 
>> 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?

This sounds nice!

If the hash depends only on the raw bits, another natural thing would be to add it
to the output of llvm-bcanalyzer somehow, or add an optional check there.

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