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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added reviewers: mgrang, craig.topper, dblaikie, RKSimon.
spatel added a comment.

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?

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