[PATCH] D72885: Add Query API for llvm.assume holding attributes

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

In D72885#1879532 <https://reviews.llvm.org/D72885#1879532>, @Tyker wrote:

> addressed comments
>
> the Name is used in the key to prevent non-deterministic order of properties in llvm.assume.
>
> the DenseSet used for uniquing, to prevent reliance on the actual hashing implementation is randomized on each run of the program. so if we don't fully order element as we do BuildAssumeFromInst can build llvm.assume where the order of constaint in the bundle may change from run to run. this is not usually visible. but this is going to be an issue for the tests as they are usually simple string matches on the output.


[Apologies for the wall of text, I wrote down my thoughts as I think it might help (in the future).]

At first this made sense to me and put my worries to rest. I agree that if we only use the names to make it deterministic but not rely on them in any other way we should be OK. That said, it is not deterministic in the absence of names, correct? I mean, if we remove the names of *all* variables, every `getName()` invocation will return the empty string. While determinism is a describable property for testing, it is crucial for compilation in general. However, I think the compilation part is not a problem right now as we can only query the extreme values (largest/lowest) for a pair `attribute + value`. In which order all the operand bundles that store a value for such a pair are internally doesn't matter, the largest/lowest is always the same. Now for testing I am also not too worried. We can test with the `CHECK-DAG` feature if we have ambiguous cases or force values to have names to make it deterministic.

Long story short, LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72885/new/

https://reviews.llvm.org/D72885