LocalCSE: Optimize idempotent functions#8383
Conversation
| ;; RUN: wasm-opt -all %s -S -o - | filecheck %s | ||
| ;; RUN: wasm-opt -all --roundtrip %s -S -o - | filecheck %s | ||
|
|
||
| ;; Test text and binary handling of @binaryen.idempotent. |
There was a problem hiding this comment.
Worth testing the call site annotation as well?
There was a problem hiding this comment.
Ah, it is actually a little tedious to check it (need to be careful to check the right one, and remember the value), so I was thinking to just not optimize it, but forgot to add a comment with a TODO. I added one now.
There was a problem hiding this comment.
Makes sense for CSE, but we could still test for the round tripping, right?
|
|
||
| bool isIdempotent(Expression* curr, Module& wasm) { | ||
| if (auto* call = curr->dynCast<Call>()) { | ||
| if (Intrinsics::getAnnotations(wasm.getFunction(call->target)).idempotent) { |
There was a problem hiding this comment.
Don't we support annotations on the call sites as well?
There was a problem hiding this comment.
(see above, sorry for the confusion)
| (i32.eq | ||
| (global.get $mutable) | ||
| (global.get $mutable) | ||
| ) |
There was a problem hiding this comment.
It might be clearer to do (i32.add (global.get $mutable) (i32.const 1)) or similar. My first thought when I saw this test was confusion because the i32.eqs are obviously optimizable - just not by LocalCSE.
There was a problem hiding this comment.
Changed to i32.add (that seems obvious enough, even without an i32.const? the const adds more stuff to read, and the need to see the constant value does not change)
2 participants
As a drive-by, add a
test/lit/idempotent.wasttest that wasmissing, for binary format testing of that hint.