As discussed in D86843, -earlycse-debug-hash should be used in more regression
tests to catch inconsistency between the hashing and the equivalence check.
The options is also changed to be enabled by default if LLVM is built with
EXPENSIVE_CHECKS defined.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
FWIW personally i'm only somewhat cautious about the compile time impact.
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.
Adding some more potential reviewers.
Ok, that sounds reasonable to me. But please wait at least a day to commit in case anyone else has comments.
llvm/test/Transforms/InstSimplify/ConstProp/math-1.ll | ||
---|---|---|
2 | 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. |
For the record, i don't know just how slower this makes EarlyCSE in EXPENSIVE_CHECKS build
I would expect "a lot" because this effectively makes EarlyCSE compare every instruction with every other instruction, i.e. quadratic in the function size. Might be too much even for EXPENSIVE_CHECKS, though I can't say I'm familiar with conventions for those.
It would be good to keep EXPENSIVE_CHECKS in a state where it can actually be used as a replacement for a normal compiler compiler if you're okay with a 10x-100x slowdown. Quadratic/cubic time growth tends to make the resulting compiler completely unusable for compiling practical code: code that contains certain patterns takes effectively forever to compile.
Yeah, the potential for quadratic explosion is why I didn't hook this flag up to expensive checks when I added it. It occurs to me now that if we want, we could presumably have some cutoff counter so that after inserting 300 instructions or whatever limit is reasonable, we start hashing normally. We'd have to empty the table and repopulate it with the normal hash when we cut over, but that should be doable I think (it's similar to what the table does internally when it grows).
I think it would be best to split this into two parts: the test changes, and potential default flip.
Removed the change to the default value of EarlyCSEDebugHash as per @lebedev.ri's suggestion.
llvm/test/Transforms/InstSimplify/ConstProp/math-1.ll | ||
---|---|---|
2 | Do you mean I should replace -early-cse with -inst-simplify in these tests? I have got rid of the whitespace-related diffs. |
llvm/test/Transforms/InstSimplify/ConstProp/math-1.ll | ||
---|---|---|
2 | If you don't mind, I would prefer to consider changing the InstSimplify tests in a separate patch. |
llvm/test/Transforms/InstSimplify/ConstProp/math-1.ll | ||
---|---|---|
2 | No problem - and yes, I think we just replace -early-cse in each RUN line for this set of tests that is checking for constant propagation. |
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.