This is an archive of the discontinued LLVM Phabricator instance.

[EarlyCSE] Verify hash code in regression tests
ClosedPublic

Authored by bryanpkc on Aug 31 2020, 1:55 AM.

Details

Summary

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.

Diff Detail

Event Timeline

bryanpkc created this revision.Aug 31 2020, 1:55 AM
bryanpkc requested review of this revision.Aug 31 2020, 1:55 AM
bryanpkc edited the summary of this revision. (Show Details)Aug 31 2020, 1:56 AM
lebedev.ri accepted this revision.EditedAug 31 2020, 2:10 AM

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?

This revision is now accepted and ready to land.Aug 31 2020, 2:10 AM

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.

lebedev.ri added a comment.EditedAug 31 2020, 5:23 AM

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

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

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.

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.

nikic added a subscriber: nikic.Aug 31 2020, 9:57 AM

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.

bryanpkc updated this revision to Diff 289788.Sep 3 2020, 12:42 PM

Removed the change to the default value of EarlyCSEDebugHash as per @lebedev.ri's suggestion.

lebedev.ri accepted this revision.Sep 3 2020, 12:45 PM

Thanks, this part still LG.

bryanpkc added inline comments.Sep 3 2020, 12:55 PM
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.

bryanpkc edited the summary of this revision. (Show Details)Sep 3 2020, 12:56 PM
bryanpkc added inline comments.Sep 4 2020, 7:35 AM
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.

This revision was automatically updated to reflect the committed changes.
spatel added inline comments.Sep 4 2020, 8:00 AM
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.