[PATCH] D86863: [EarlyCSE] Verify hash code in regression tests

Mon Aug 31 05:23:17 PDT 2020

lebedev.ri added a comment.

FWIW personally i'm only somewhat cautious about the compile time impact.

In D86863#2247283 <https://reviews.llvm.org/D86863#2247283>, @spatel wrote:

> In D86863#2247060 <https://reviews.llvm.org/D86863#2247060>, @lebedev.ri wrote:
>
>> Thanks!
>>
>> Test changes are certainly LGTM, may be best to commit them separately.
>>
>> For the record, i don't know just how slower this makes EarlyCSE in `EXPENSIVE_CHECKS` build,
>> but i can't imagine having lurking nondeterminism with an easy way to catch it is better.
>> @spatel thoughts on the switch?
>
> I also don't know how to detect the underlying bug (inconsistent hashing) in a better way. EXPENSIVE_CHECKS sounds like the right enabling flag to me.
>
> Whether we should do this as a part of normal regression testing, I'm not sure. I know of 1 other example of normalizing an expensive testing scenario by explicitly setting a flag in RUN lines: targets like PowerPC set "-verify-machineinstr" on all CodeGen tests for better coverage.

> But IIUC, that does extra checking without altering the normal codepaths. Changing the debug hash is different because we are no longer testing the underlying behavior that a normal user would experience. Are we creating a testing blind spot by changing these tests?

This is a non-issue, because these tests aren't tests for DenseMap,
and the only thing that flag does, this causes all hashes to collide,
thus forcing DenseMap to call `DenseMapInfo<SimpleValue>::isEqual()`,
that verifies this invariant.
So this causes strictly larger code coverage footprint, not smaller.

> Adding some more potential reviewers.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86863