Page MenuHomePhabricator

Make precompiled headers reproducible by switching OpenCL extension to std::map
AbandonedPublic

Authored by riccibruno on Apr 16 2019, 8:50 AM.

Details

Summary

As discussed in https://reviews.llvm.org/D60379 let's use std::map to store OpenCL extensions data structure.

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

Diff Detail

Event Timeline

Anastasia created this revision.Apr 16 2019, 8:50 AM

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Ok I could do that, but I guess it can then fail on the commits that didn't actually trigger the issue. Would it not be a problem?

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Ok I could do that, but I guess it can then fail on the commits that didn't actually trigger the issue. Would it not be a problem?

Why would it fail if this fixes the issue?
Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv

Mmm. I hope I am not missing something obvious, but how is this actually fixing the issue ? I don't see why the order between the Type * and between the Decl * should be deterministic (I think they will be when they are part of the same slab in the allocator, but I don't see why the slab ordering should be stable).

Mmm. I hope I am not missing something obvious, but how is this actually fixing the issue ? I don't see why the order between the Type * and between the Decl * should be deterministic (I think they will be when they are part of the same slab in the allocator, but I don't see why the slab ordering should be stable).

Oh. Duh.
Can a custom comparator be provided for these std::map, equivalent to the one in https://reviews.llvm.org/D60379 ?
Something like getTypeID(LHS.first->getCanonicalTypeInternal()) < getTypeID(RHS.first->getCanonicalTypeInternal()) ?

Maybe, but is there an equivalent to getTypeID for declarations ?

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Ok I could do that, but I guess it can then fail on the commits that didn't actually trigger the issue. Would it not be a problem?

Why would it fail if this fixes the issue?

But once something is changed and it's no longer deterministic, we might not be able to detect this necessarily on the offending commit with such test?

Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv

Maybe, but is there an equivalent to getTypeID for declarations ?

getDeclID is used in the first patch...

although now I am thinking may be original patch is a better approach?

What about something like D60835 ?

Abandon this?
Sorry for this fruitless effort.

riccibruno commandeered this revision.Apr 23 2019, 9:29 AM
riccibruno edited reviewers, added: Anastasia; removed: riccibruno.

Closing this since the issue was fixed in r358674.

riccibruno abandoned this revision.Apr 23 2019, 9:30 AM