Add pass to instrument memory.grow instructions #7388
Conversation
0ee3162 to
68f0669
Compare
|
Those use cases make sense to me. How about this?
|
|
Thanks @kripken for feedback. I don't mind updating the I don't insist on any of the options though, and I'd be happy to leave the decision to you. |
|
@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. |
|
Yes, that's exactly what I meant. I'll update the PR soon. |
68f0669 to
b9bf2ee
Compare
93f551c to
2dc6c17
Compare
2dc6c17 to
fadb6f2
Compare
| @@ -406,6 +421,8 @@ | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $A (type $1) | ||
| (drop (memory.grow (i32.const 4))) |
There was a problem hiding this comment.
Looks like this is still an issue - ping me for review when it is ready.
There was a problem hiding this comment.
Could you clarify what's the issue here? Is it about lack of comment? I don't mind adding it, but in that case should the comment be added to all the other instructions in this file, as we don't specify any filter so all the memory operations are instrumented.
There was a problem hiding this comment.
Sorry, I should have linked to my other comment that this was a followup to,
Maybe add this at the end, not the start? That would make the diff far shorter.
It might be even better in a new function, for easier reading.
(link)
My concern is the location. By putting this new test at the top, look at how large the diff is. All the existing tests end up modified. I am suggesting you add the new stuff at the bottom, to minimize the diff.
There was a problem hiding this comment.
Oh you're right, updated that.
Also added instruction filter that allows to limit instrumented instructions.
fadb6f2 to
4d8a838
Compare
…yen (#24467) This was added in WebAssembly/binaryen#7388
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.