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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D86863#2247308 <https://reviews.llvm.org/D86863#2247308>, @lebedev.ri wrote:

>> 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, is 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.

Ok, that sounds reasonable to me. But please wait at least a day to commit in case anyone else has comments.



================
Comment at: llvm/test/Transforms/InstSimplify/ConstProp/math-1.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -early-cse -earlycse-debug-hash -S -o - %s | FileCheck %s
+
----------------
Several tests like this are in the InstSimplify test directory, but not using -instsimplify directly. I think it would be better to update them to not use -early-cse at all.

Also, if there is some whitespace issue causing the entire file to update here, it would be better to fix that as a preliminary cleanup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86863