Page MenuHomePhabricator

[LegacyPM] Double check that passes correctly set their Modified status
ClosedPublic

Authored by serge-sans-paille on Jun 1 2020, 3:29 AM.

Details

Summary

The approach is simple: if a pass reports that it's not modifying a
Function/Module, compute a loose hash of that Function/Module and compare it
with the original one. If we report no change but there's a hash change, then we
have an error.

This approach misses a lot of change but it's not super intrusive and can
detect most of the simple mistakes.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 3:29 AM
foad added inline comments.Jun 1 2020, 4:10 AM
llvm/lib/IR/LegacyPassManager.cpp
1499

This doesn't even compile because the definition of FunctionHash has a capital F.

1557

This is the wrong way round. You want to assert that LocalChanged || RefHash == FunctionHash(F).

Fixed and tested implementation

serge-sans-paille marked 2 inline comments as done.Jun 1 2020, 8:51 AM

Sorry for uploading the previous patch. This one should be OK.

foad added inline comments.Jun 1 2020, 9:45 AM
llvm/lib/IR/LegacyPassManager.cpp
1668

==

foad added a comment.Jun 3 2020, 8:46 AM

I like this and it is certainly useful for finding bugs. I'd like to see it for Loop, Region and SCC passes too. But I don't know if it's fast and/or robust enough to commit and have it enabled as-is.

Is there any other code for hashing Functions and Modules that we could use here instead of rolling our own?

I like this and it is certainly useful for finding bugs. I'd like to see it for Loop, Region and SCC passes too. But I don't know if it's fast and/or robust enough to commit and have it enabled as-is.

It currently finds a bunch of bugs (passes not correctly reporting modification), I'm trying to fix them iteratively :-)

Is there any other code for hashing Functions and Modules that we could use here instead of rolling our own?

The code is ostly borrowed from the transform utils. We can't have Core depend on Transform, but it's 100% possible to moe them around. I'll do that and update the patch.

nikic added a comment.Jun 3 2020, 9:24 AM

The code is ostly borrowed from the transform utils. We can't have Core depend on Transform, but it's 100% possible to moe them around. I'll do that and update the patch.

I'm not sure it makes sense to share this code. The hashing in the transform utils is used for function merging, and as such is intentionally rather weak (in the sense that it will let many functions that are different hash to the same value, as the notion of equality in that pass is not that strict). This code on the other hand could hash in more information, to catch more possible issues. (It doesn't have to, but it wouldn't be incorrect to do so.)

dmajor added a subscriber: dmajor.Jun 4 2020, 9:01 AM

Use EXPENSIVE_CHECKS.
Fix a test case that was incorrectly adding instruction after the terminator.

I think having a separate hashing here is fine as we will most likely diverge as well. We should, for example, check attributes here to at some point (maybe worth a FIXME). Other than that, I think this looks good.

@nikic I let you finish your review :)

All the reviews required to land this without breaking the validation have landed, @jdoerfert / @nikic / @foad I'd be super-happy to land this one before branching.

foad added inline comments.Jul 8 2020, 1:49 AM
llvm/unittests/IR/LegacyPassManagerTest.cpp
683

Is this change related to the rest of the patch somehow?

serge-sans-paille marked an inline comment as done.Jul 8 2020, 3:34 AM
serge-sans-paille added inline comments.
llvm/unittests/IR/LegacyPassManagerTest.cpp
683

Yes, without this change, the call is added at the end of the entryblock, i.e. after the terminator. It happens that hash computation triggers an assert (indirectly) on that (bad) situation.

foad added a comment.Jul 8 2020, 3:48 AM

Looks good to me apart from the comments inline, but I'd like someone else to approve it too.

As a follow-up can we have the same checking for Loop, Region and SCC passes?

llvm/lib/IR/LegacyPassManager.cpp
1492

I don't like having both functionHash and FunctionHash. How about having FunctionHash take the initial hash value as an optional argument that defaults to StructuralHash() ?

Take final review into account

jdoerfert accepted this revision.Jul 8 2020, 8:04 AM

LGTM. Thanks for all the hard work on making this possible :)

This revision is now accepted and ready to land.Jul 8 2020, 8:04 AM
This revision was automatically updated to reflect the committed changes.

Note: I had to revert this becasue I only tested X86 targe, and other targets suffer from a lot of « don't update return status » error. cc @fhahn

fhahn added a comment.Jul 9 2020, 2:00 AM

Note: I had to revert this becasue I only tested X86 targe, and other targets suffer from a lot of « don't update return status » error. cc @fhahn

Hmm, where do other targets suffer from those errors? In the various backend pipelines/passes?

Hmm, where do other targets suffer from those errors? In the various backend pipelines/passes?

Yes, the culprits where https://reviews.llvm.org/D83457 https://reviews.llvm.org/D83459 and https://reviews.llvm.org/D83460

fhahn added a comment.Jul 9 2020, 2:36 AM

Hmm, where do other targets suffer from those errors? In the various backend pipelines/passes?

Yes, the culprits where https://reviews.llvm.org/D83457 https://reviews.llvm.org/D83459 and https://reviews.llvm.org/D83460

Sounds good. I can try a build on AArch64 in a bit.

Cross-compiling the test-suite for example should be relatively straight-forward, if you have a linker & libraries for the target architecture (e.g. on linux it should be easy to get the required toolchains for platforms like ARM and AArch64) https://llvm.org/docs/lnt/tests.html#cross-compiling

Sounds good. I can try a build on AArch64 in a bit.

Cross-compiling the test-suite for example should be relatively straight-forward, if you have a linker & libraries for the target architecture (e.g. on linux it should be easy to get the required toolchains for platforms like ARM and AArch64) https://llvm.org/docs/lnt/tests.html#cross-compiling

Well, I can trigger the failure locally, it's just that I didn't enable all LLVM_TARGETS during my local builds. I hope cross compiling won't unveil more issues :-)