Page MenuHomePhabricator

[Serialization] Stable serialization order for OpenCLTypeExtMap and OpenCLDeclExtMap
ClosedPublic

Authored by riccibruno on Apr 17 2019, 2:36 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

riccibruno created this revision.Apr 17 2019, 2:36 PM

By the way, I am wondering about how much this is tested. I did look quickly in test/PCH and it appears that there are only 3 (short) tests : ocl_types.cl, opencl-extensions.cl and no-validate-pch.

By the way, I am wondering about how much this is tested. I did look quickly in test/PCH and it appears that there are only 3 (short) tests : ocl_types.cl, opencl-extensions.cl and no-validate-pch.

This is tested in test/SemaOpenCL/extension-begin.cl: https://reviews.llvm.org/D53200

The ordering in PCH isn't tested anywhere however, but I am not clear how to check for nondeterminism properly. Because the fact that the same output is generated N times won't guarantee that it's the same N+1 times. Especially I do believe it's likely on the same revision the same output to be produced but it's less likely across revisions. :(

We could still go for something like the following but accept that some a random failure might happen not necessarily on a commit that introduces it?

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.

By the way, I am wondering about how much this is tested. I did look quickly in test/PCH and it appears that there are only 3 (short) tests : ocl_types.cl, opencl-extensions.cl and no-validate-pch.

This is tested in test/SemaOpenCL/extension-begin.cl: https://reviews.llvm.org/D53200

Ah I see, I did indeed miss that.

The ordering in PCH isn't tested anywhere however, but I am not clear how to check for nondeterminism properly. Because the fact that the same output is generated N times won't guarantee that it's the same N+1 times. Especially I do believe it's likely on the same revision the same output to be produced but it's less likely across revisions. :(

We could still go for something like the following but accept that some a random failure might happen not necessarily on a commit that introduces it?

I am not sure that this is needed. Non-deterministic tests are really annoying.

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.

Anastasia accepted this revision.Apr 18 2019, 6:57 AM

We could still go for something like the following but accept that some a random failure might happen not necessarily on a commit that introduces it?

I am not sure that this is needed. Non-deterministic tests are really annoying.

Yes. I think this might be a wider topic to address if we happen to have more issues of this kind.

This revision is now accepted and ready to land.Apr 18 2019, 6:57 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2019, 8:14 AM