Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pass to instrument memory.grow instructions #7388

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

loganek
Copy link
Contributor

@loganek loganek commented Mar 21, 2025

I've implemented this functionality as a new pass, however, it could also be an extension of the existing instrument-memory pass. The main reason for that is because I'd like to be able to use this instrumentation in production code, not just for debugging - the instrument-memory pass adds lot more instrumentation which would impact the performance of my code.

Alternatively, I could extend the instrument-memory pass and add a parameter to select instructions that should be instrumented - I'm open for feedback; if that's preferred approach, I'd be happy to update the PR.

@loganek loganek force-pushed the loganek/memory-grow-instrument branch from 0ee3162 to 68f0669 Compare March 21, 2025 09:21
@kripken
Copy link
Member

kripken commented Mar 21, 2025

Those use cases make sense to me. How about this?

  1. Make the existing InstrumentMemory pass also instrument grows - it should do everything basically.
  2. Add a new pass that just instruments grows, which is what you mainly want.
  3. Can maybe put them both in the same file to save a little code.

@loganek
Copy link
Contributor Author

loganek commented Mar 24, 2025

Thanks @kripken for feedback. I don't mind updating the InstrumentMemory to include the memory.grow tracking too. Having said that, I wonder if it makes sense to keep the InstrumentMemoryGrow pass at all, and instead just add some an optional enabled-instructions parameter to the InstrumentMemory pass that allows for choosing which instructions to pick? That'd be a bit more generic and we'd avoid duplicating the same functionality for different passes.

I don't insist on any of the options though, and I'd be happy to leave the decision to you.

@kripken
Copy link
Member

kripken commented Mar 24, 2025

@loganek A pass argument is a nice idea. So by default it instruments everything, but you can pick an explicit subset? That sounds good to me.

@loganek
Copy link
Contributor Author

loganek commented Mar 24, 2025

Yes, that's exactly what I meant. I'll update the PR soon.

@loganek loganek force-pushed the loganek/memory-grow-instrument branch from 68f0669 to b9bf2ee Compare March 25, 2025 11:10
;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up.

;; RUN: foreach %s %t wasm-opt --instrument-memory="i32.load,memory.grow" -S -o - | filecheck %s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining that only i32.load and memory.grow should be instrumented below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the comment - where is it?

The first function should have a comment "we instrument grow and loads because we were asked to by the flags. The second, "we do not instrument store since we were not asked to by the flags".

@loganek loganek force-pushed the loganek/memory-grow-instrument branch 2 times, most recently from 93f551c to 2dc6c17 Compare April 2, 2025 22:54
;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up.

;; RUN: foreach %s %t wasm-opt --instrument-memory="i32.load,memory.grow" -S -o - | filecheck %s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the comment - where is it?

The first function should have a comment "we instrument grow and loads because we were asked to by the flags. The second, "we do not instrument store since we were not asked to by the flags".

Also added instruction filter that allows to limit instrumented instructions.
@loganek loganek force-pushed the loganek/memory-grow-instrument branch from 2dc6c17 to fadb6f2 Compare April 3, 2025 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants