[SandboxIR] Add callbacks for instruction insert/remove/move ops #112965

slackito · 2024-10-18T19:56:28Z

No description provided.

tschuett · 2024-10-18T19:59:52Z

llvm/include/llvm/SandboxIR/Context.h

@@ -48,6 +61,21 @@ class Context {
  /// Type objects.
  DenseMap<llvm::Type *, std::unique_ptr<Type, TypeDeleter>> LLVMTypeToTypeMap;

+  /// Callbacks called when an IR instruction is about to get removed. Keys are
+  /// used as IDs for deregistration.
+  DenseMap<int, RemoveInstrCallback> RemoveInstrCallbacks;


Do you want to keep them as ints or:

using ID = int;

?

Good point, thanks! Switched it to CallbackID.

tschuett · 2024-10-18T20:00:11Z

llvm/lib/SandboxIR/Context.cpp

+}
+
+void Context::runInsertInstrCallbacks(Instruction *I) {
+  for (auto &CBEntry : InsertInstrCallbacks) {


unnecessary braces

github-actions · 2024-10-18T20:00:22Z

✅ With the latest revision this PR passed the C/C++ code formatter.

tschuett · 2024-10-18T20:00:25Z

llvm/lib/SandboxIR/Context.cpp

@@ -660,4 +663,57 @@ Module *Context::createModule(llvm::Module *LLVMM) {
  return M;
 }

+void Context::runRemoveInstrCallbacks(Instruction *I) {
+  for (const auto &CBEntry : RemoveInstrCallbacks) {


unnecessary braces

tschuett · 2024-10-18T20:00:38Z

llvm/lib/SandboxIR/Context.cpp

+}
+
+void Context::runMoveInstrCallbacks(Instruction *I, const BBIterator &WhereIt) {
+  for (auto &CBEntry : MoveInstrCallbacks) {


unnecessary braces

- Introduced `CallbackID` typedef for callback ids rather than a plain int. - Remove unnecessary braces.

aeubanks

ultra nit: no period at end of commit title

llvm/include/llvm/SandboxIR/Context.h

aeubanks · 2024-10-18T20:59:26Z

llvm/lib/SandboxIR/Context.cpp

+  return ID;
+}
+void Context::unregisterInsertInstrCallback(CallbackID ID) {
+  [[maybe_unused]] bool erased = InsertInstrCallbacks.erase(ID);


Suggested change

[[maybe_unused]] bool erased = InsertInstrCallbacks.erase(ID);

[[maybe_unused]] bool Erased = InsertInstrCallbacks.erase(ID);

- Updated callback (de)registration method signatures in header to use the CallbackID type instead of int. - Corrected case in some variable names to follow the style guide.

vporpo · 2024-10-18T21:12:27Z

llvm/include/llvm/SandboxIR/Context.h

+      std::function<void(Instruction *, const BBIterator &)>;
+
+  /// An ID for a registered callback. Used for deregistration.
+  using CallbackID = int;


Why use an ID and not the raw pointer of the callback itself as the ID ?

non-determinism

Pointers to the std::function objects stored in a map/vector can get invalidated when registering/unregistering more callbacks, right?

How are we going to get non-determinism ? The ID itself is not printed anywhere, it's just used as a key to remove the callback if needed. If it's the raw pointer of the function it should still work just as well. Or am I missing something?

Pointers to the std::function objects stored in a map/vector can get invalidated when registering/unregistering more callbacks, right?

True but we can get around that in multiple ways, one of which is by allocating unique pointers and getting the raw pointer.

What I don't like about the integer IDs is that:

they are just being used as a key and they don't really convey anything else. They are not even being used for debugging purposes.

they need the static? counter

How is another layer of indirection and an extra heap allocation better than keeping an integer around?

If you use raw pointers as keys in maps, you will get non-determinism.

The context should own the counter.

How is another layer of indirection and an extra heap allocation better than keeping an integer around?

Yeah, it's definitely not great, and we should try to make the code that runs the callbacks as fast as possible.

Since @tschuett brought up non-determinism, should we try to make sure that the callbacks are run in the same order as registered? Because we are currently not only not getting that order but since we are iterating over the DenseMap we are also getting a non-deterministic order. Using a MapVector will fix non-determinism, but perhaps we should try to stick to registration order, and try to change it later if it turns out that it's not needed. I think we could use a vector instead of a map, because removal of a callback shouldn't happen too often (and we won't have too many callbacks) so a linear-time search should be fine. Wdyt?

tmsri · 2024-10-18T23:30:02Z

llvm/unittests/SandboxIR/SandboxIRTest.cpp

+  EXPECT_THAT(Inserted, testing::ElementsAre(NewI));
+  EXPECT_THAT(Removed, testing::IsEmpty());
+  EXPECT_THAT(Moved, testing::IsEmpty());
+


A drive-by comment: How about testing insertBefore and insertAfter?

Discussed this offline because @vporpo's prototype only called them on create/erase, but called them insert/remove. We only need create/erase/move callbacks for the vectorizer, so we're only adding those for now, and I have renamed them to create/erase callbacks. This leaves space to add proper insert/remove callbacks in the future if we (or some future user of Sandbox IR) needs them.

Thus, no need to test those here as they have no associated callbacks for now.

Added test to check that registration order is respected when invoking registered callbacks.

- Remove insert/remove callbacks to create/erase. Don't call the erase callback on remove. This way we have a consistent model, but we don't have insert/remove callbacks, just create/erase/move. The missing callbacks can still be added if needed in the future. - Changed callback ids to uint64 in the unlikely case they could overflow a 32-bit integer in a large compile, causing hard-to-debug errors.

slackito · 2024-10-26T00:29:56Z

Please take another look. I have also switched callback ids from int to uint64_t out of concerns (expressed offline) that we could possibly accidentally overflow a 32-bit integer (maybe registering/deregistering callbacks multiple times per region in a very large compile or something like that).

vporpo

Looks good to me.
I think it would make sense to add some assertions on the callback map size.

vporpo · 2024-10-27T00:29:21Z

llvm/lib/SandboxIR/Context.cpp

+
+Context::CallbackID Context::registerEraseInstrCallback(EraseInstrCallback CB) {
+  CallbackID ID = NextCallbackID++;
+  EraseInstrCallbacks[ID] = CB;


How about an assertion on EraseInstrCallbacks.size() checking that it's smaller than say 16 ? It could help catch issues where you keep on registering callbacks by mistake.

Done. I'm not super happy with having an arbitrary limit like this not documented anywhere and with a kinda low value, but it will be useful while we're developing the vectorizer and we can always revisit this decision later.

…m#112965)

[SandboxIR] Add callbacks for instruction insert/remove/move ops.

56ffef4

slackito requested review from alinas, vporpo, tschuett, aeubanks, tmsri and Sterling-Augustine October 18, 2024 19:56

tschuett reviewed Oct 18, 2024

View reviewed changes

Address some review feedback.

f361d5c

- Introduced `CallbackID` typedef for callback ids rather than a plain int. - Remove unnecessary braces.

aeubanks reviewed Oct 18, 2024

View reviewed changes

slackito changed the title ~~[SandboxIR] Add callbacks for instruction insert/remove/move ops.~~ [SandboxIR] Add callbacks for instruction insert/remove/move ops Oct 18, 2024

Another round of feedback.

e84e9e6

- Updated callback (de)registration method signatures in header to use the CallbackID type instead of int. - Corrected case in some variable names to follow the style guide.

vporpo reviewed Oct 18, 2024

View reviewed changes

Make NextCallbackID not static

fa875d7

tmsri reviewed Oct 18, 2024

View reviewed changes

slackito added 3 commits October 18, 2024 18:07

Switched callbacks to MapVector to iterate in registration order.

9068541

Added test to check that registration order is respected when invoking registered callbacks.

clang-format

35a2074

vporpo approved these changes Oct 27, 2024

View reviewed changes

slackito added 2 commits October 28, 2024 18:17

Add assertion for max callbacks registered at once

367e5b5

more clang-format

3ef32de

slackito merged commit 4df71ab into llvm:main Oct 29, 2024
8 checks passed

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024

[SandboxIR] Add callbacks for instruction insert/remove/move ops (llv…

17df55e

…m#112965)

	[[maybe_unused]] bool erased = InsertInstrCallbacks.erase(ID);
	[[maybe_unused]] bool Erased = InsertInstrCallbacks.erase(ID);

[SandboxIR] Add callbacks for instruction insert/remove/move ops #112965

[SandboxIR] Add callbacks for instruction insert/remove/move ops #112965

Uh oh!

Conversation

slackito commented Oct 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vporpo Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slackito commented Oct 26, 2024

Uh oh!

vporpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2024 •

edited

Loading

vporpo Oct 18, 2024 •

edited

Loading