[SandboxIR] IR Tracker #99238

vporpo · 2024-07-16T20:46:48Z

This is the first patch in a series of patches for the IR change tracking component of SandboxIR.
The tracker collects changes in a vector of IRChangeBase objects and provides a save()/accept()/revert() API.

Each type of IR changing event is captured by a dedicated subclass of IRChangeBase. This patch implements only one of them, that for updating a sandboxir::Use source value, named UseSet.

llvm/include/llvm/SandboxIR/SandboxIR.h

llvm/docs/SandboxIR.md

llvm/include/llvm/SandboxIR/SandboxIRTracker.h

llvm/lib/SandboxIR/SandboxIRTracker.cpp

llvm/include/llvm/SandboxIR/SandboxIRTracker.h

llvm/docs/SandboxIR.md

aeubanks · 2024-07-17T18:21:56Z

llvm/include/llvm/SandboxIR/Tracker.h

+class IRChangeBase {
+protected:
+#ifndef NDEBUG
+  unsigned Idx = 0;


add a comment about Idx

actually, if it's only used for debugging, why can't we print the index when iterating over the change vector?

Yes we could do that. In an early implementation there was no Parent pointer, so Idx had to be a member of the change class.

I meant if we're dumping the entire vector of changes, can't we do something like

for (auto [C, Idx]: enumerate(changes)) { errs() << Idx << ": " << C << "\n"; }

so no need for IRChangeBase to know anything about its index

We could do that, but wouldn't it be nice to also get the index when we dump a specific change object? This could be useful for example when a revert() crashes and we need to figure out which of the changes is this one (there may be many of them of the same class).

if you've found that useful in the past, then sure

aeubanks · 2024-07-17T18:23:06Z

llvm/include/llvm/SandboxIR/Tracker.h

+  UseSet(const Use &U, SandboxIRTracker &Tracker)
+      : IRChangeBase(TrackID::UseSet, Tracker), U(U), OrigV(U.get()) {}
+  // For isa<> etc.
+  static bool classof(const IRChangeBase *Other) {


what is this used for? it doesn't really make sense to have both virtual inheritance and isa<> on a class

Ah I think this is dead code. I think it was meant to be used for fast upcasting, but I think it's not used anywhere. I will remove it. Thanks for noticing.

Hmm now that classof() is not needed, the only use of the TrackID should be for dumping the class name when debugging. This could be done by simply passing the class name to the parent constructor like:

UseSet(const Use &U, Tracker &Tracker) : IRChangeBase("UseSet", &Parent: Tracker), ...

But ideally we should only use the name in the debug build.
What is a good way of #ifdefing it? Should I use two separate constructors? or one constructor with an #ifdefed argument like:

UseSet(const Use &U, Tracker &Tracker) : IRChangeBase( #ifndef NDEBUG "UseSet", #endif // NDEBUG &Parent: Tracker), ...

do you expect that this is actually going to get used? can we just drop it?

i.e. how often is this actually going to help with debugging an issue? and how often are issues around this going to appear?

in general having too many differences between a NDEBUG and non-NDEBUG build makes it more annoying to develop and harder to catch issues that only arise in one configuration. I'd lean toward simplifying when possible and only adding debugging aids when the types of issues it helps debug come up often enough and the debugging aid actually makes it easier to debug

When there is a bug in checkpointing it is useful to get a dump and see which changes are in the Changes vector and look if any of them looks suspicious. Without it you will just have to guess.

Another option is to keep it in the prod build for now and remove it later once the whole infrastructure is stable.

can you manually print the string in the dump() override? instead of keeping something as a member variable

Yeah, that works too.

aeubanks · 2024-07-17T20:23:20Z

llvm/docs/SandboxIR.md

+Internally this will go through the changes and run any finalization required.
+
+Please note that after a call to `revert()` or `accept()` tracking will stop.
+So the user would need to start it again if needed with a call to `save()`.


Suggested change

So the user would need to start it again if needed with a call to `save()`.

To start tracking again, the user needs to call `save()`.

aeubanks · 2024-07-17T20:27:19Z

llvm/include/llvm/SandboxIR/Tracker.h

+class IRChangeBase {
+protected:
+#ifndef NDEBUG
+  unsigned Idx = 0;


actually, if it's only used for debugging, why can't we print the index when iterating over the change vector?

aeubanks · 2024-07-17T20:28:55Z

llvm/include/llvm/SandboxIR/Tracker.h

+
+#ifndef NDEBUG
+  /// \Returns the \p Idx'th change. This is used for testing.
+  IRChangeBase *getChange(unsigned Idx) const { return Changes[Idx].get(); }


this seems like testing implementation details rather than testing behavior. can we just test observable IR accept/revert behavior rather than the test verifying individual IRChangeBases

Yeah we can drop this.

aeubanks · 2024-07-17T23:18:32Z

llvm/include/llvm/SandboxIR/Tracker.h

+    Revert,   ///> Undoing changes
+    Accept,   ///> Accepting changes


are these states ever used for anything? is there anything to gain from setting the state to Revert/Accept when reverting/accepting (like InMiddleOfCreatingChange)?

They are, but let's remove them for now and we can add them when they are actually needed. There may be a way to do without them.

We are using the same IR API functions from within the body of the revert() functions so we need a way to tell whether we are reverting or not, but perhaps the Disabled state is good enough.

aeubanks · 2024-07-17T23:22:29Z

llvm/unittests/SandboxIR/TrackerTest.cpp

+
+  // Check RUWIf when the lambda returns true.
+  Ld0->replaceUsesWithIf(Ld1, [](const sandboxir::Use &Use) { return true; });
+  EXPECT_EQ(Tracker.size(), 2u);


this seems like testing implementation details, I'm not sure it's necessary to test this

OK we can do without this.

aeubanks · 2024-07-17T23:22:44Z

llvm/unittests/SandboxIR/TrackerTest.cpp

+  LLVMContext C;
+  std::unique_ptr<Module> M;
+
+  void parseIR(LLVMContext &C, const char *IR) {


these helper methods are in almost every unittest, we really need these common utilities for all unittests somewhere shared (if they don't already exist), but that's a problem for another day

Yeah there is a lot of replication.

aeubanks · 2024-07-18T03:52:21Z

llvm/unittests/SandboxIR/TrackerTest.cpp

+  sandboxir::Instruction *St1 = &*It++;
+  Ctx.save();
+  // Check RUWIf when the lambda returns false.
+  Ld0->replaceUsesWithIf(Ld1, [](const sandboxir::Use &Use) { return false; });


nothing is getting checked after this RUWI?

aeubanks · 2024-07-18T03:54:09Z

llvm/unittests/SandboxIR/TrackerTest.cpp

+  EXPECT_EQ(St0->getOperand(0), Ld1);
+  EXPECT_EQ(St1->getOperand(0), Ld1);


these are sorta redundant with the other unittests, I'd argue we only need to check after the accept/revert

Well, in the other tests the tracker is not recording, so there is a chance that they are not executing the same code.

This is the first patch in a series of patches for the IR change tracking component of SandboxIR. The tracker collects changes in a vector of `IRChangeBase` objects and provides a `save()`/`accept()`/`revert()` API. Each type of IR changing event is captured by a dedicated subclass of `IRChangeBase`. This patch implements only one of them, that for updating a `sandboxir::Use` source value, named `UseSet`.

llvm-ci · 2024-07-18T07:28:20Z

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/287

Here is the relevant piece of the build log for the reference:

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/39/86' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-18504-39-86.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=86 GTEST_SHARD_INDEX=39 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************

This is the first patch in a series of patches for the IR change tracking component of SandboxIR. The tracker collects changes in a vector of `IRChangeBase` objects and provides a `save()`/`accept()`/`revert()` API. Each type of IR changing event is captured by a dedicated subclass of `IRChangeBase`. This patch implements only one of them, that for updating a `sandboxir::Use` source value, named `UseSet`.

Summary: This is the first patch in a series of patches for the IR change tracking component of SandboxIR. The tracker collects changes in a vector of `IRChangeBase` objects and provides a `save()`/`accept()`/`revert()` API. Each type of IR changing event is captured by a dedicated subclass of `IRChangeBase`. This patch implements only one of them, that for updating a `sandboxir::Use` source value, named `UseSet`. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251568

vporpo requested review from slackito, echristo, alinas, tschuett, aeubanks and tmsri July 16, 2024 20:46

vporpo force-pushed the SBVec branch 2 times, most recently from a9d3476 to 5e8c1c4 Compare July 16, 2024 21:31

aeubanks reviewed Jul 17, 2024

View reviewed changes

tschuett reviewed Jul 17, 2024

View reviewed changes

llvm/include/llvm/SandboxIR/SandboxIRTracker.h Outdated Show resolved Hide resolved

aeubanks reviewed Jul 17, 2024

View reviewed changes

vporpo force-pushed the SBVec branch from f26c90d to 2362181 Compare July 18, 2024 02:49

aeubanks approved these changes Jul 18, 2024

View reviewed changes

vporpo force-pushed the SBVec branch from 2362181 to 8031e55 Compare July 18, 2024 04:33

vporpo merged commit 5338bd3 into llvm:main Jul 18, 2024
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SandboxIR] IR Tracker #99238

[SandboxIR] IR Tracker #99238

vporpo commented Jul 16, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

aeubanks Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

aeubanks Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 17, 2024

vporpo Jul 17, 2024

aeubanks Jul 18, 2024

vporpo Jul 18, 2024

aeubanks Jul 18, 2024

vporpo Jul 18, 2024

llvm-ci commented Jul 18, 2024

	So the user would need to start it again if needed with a call to `save()`.
	To start tracking again, the user needs to call `save()`.

		EXPECT_EQ(St0->getOperand(0), Ld1);
		EXPECT_EQ(St1->getOperand(0), Ld1);

[SandboxIR] IR Tracker #99238

[SandboxIR] IR Tracker #99238

Conversation

vporpo commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvm-ci commented Jul 18, 2024