[SandboxIR] Add debug checker to compare IR before/after a revert #115968
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will help us catch mistakes in change tracking. It's only enabled when EXPENSIVE_CHECKS are enabled.
slackito
requested review from
alinas,
vporpo,
tschuett,
aeubanks,
tmsri and
Sterling-Augustine
vporpo
reviewed
Nov 13, 2024
| void expectNoDiff(); | ||
|
|
||
| /// Empties saved state. | ||
| void reset(); |
There was a problem hiding this comment.
nit: perhaps clear() would be a better name?
Also this doesn't seem to be used. My guess is that it should be called before we save(), but isn't it better to make this private and call it from within the body of save() ?
There was a problem hiding this comment.
No need to call it from save, as it will replace the old snapshot with the new one. I've just removed it.
| // True if save() was previously called. This helps us distinguish between | ||
| // "expectNoDiff was called without calling save" and "save was called but | ||
| // the saved snapshot is empty". | ||
| bool HasSavedState = false; |
There was a problem hiding this comment.
Do we need this variable? Can't we use OrigContextSnapshot.empty() ?
There was a problem hiding this comment.
The reason, as the comment above says, is that I think the snapshot could be empty. What if we do something like:
sandboxir::Context Ctx;
Ctx.save(); // try to save without creating any modules or functions
Ctx.revert();
Sure, doing that is not useful, but it doesn't strike me as incorrect usage of the API either.
There was a problem hiding this comment.
But if we have nothing to check, shouldn't expectNoDiff() pass anyway?
If you are trying to check that we ran save() before running revert() then I think this check shouldn't be the job of the IRSnapshotChecker.
There was a problem hiding this comment.
I was trying to check we ran save() before expectNoDiff (that is, an invariant at the IRSnapshotChecker level, not at the Context level). But removing it also works, so I just did that.
llvm/lib/SandboxIR/Tracker.cpp
Outdated
| if (OrigMS.Hash != CurrMS.Hash) { | ||
| DifferenceFound = true; | ||
| dbgs() << "Found IR difference in module " << M->getName() << ".\n"; | ||
| dbgs() << "Original:\n" << OrigMS.TextualIR << "\n"; |
There was a problem hiding this comment.
This is good for now, but my concern is that these module dumps may become really huge when compiling large applications, so getting these textual dumps will become problematic. Could we perhaps do a per-function check, or per-BB check instead?
There was a problem hiding this comment.
Switched to per-function snapshots. I looked into using BBs instead but StructuralHash only takes Module or Function.
| Checker.save(); | ||
| Add1->setOperand(1, Add0); | ||
| EXPECT_DEATH(Checker.expectNoDiff(), "Found IR difference"); | ||
| } |
There was a problem hiding this comment.
perhaps we should also exercise running save() again (or reset() + save()) after some changes are made?
There was a problem hiding this comment.
Added a test case that runs save() twice and checks the second time replaces the first one.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/11282 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/8371 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will help us catch mistakes in change tracking. It's only enabled when EXPENSIVE_CHECKS are enabled.