[StackIR] Drop a set/get pair across a symmetric operation - #9162
artemkulyk wants to merge 1 commit into
Conversation
1e4946a to
f259b19
Compare
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Nice! Just a couple comments.
|
|
||
| // Whether a stack instruction is a binary operation on integers that gives the | ||
| // same result whichever operand order it is computed in. | ||
| static bool isCommutativeIntBinary(StackInst* inst) { |
There was a problem hiding this comment.
Please move this to src/ir/properties.h.
There was a problem hiding this comment.
Done. Added an integer-only Properties::isSymmetricInt and made isSymmetric use it. Kept it integer-only because swapping float min/max operands can change NaN bits.
| auto setIndex = values[j]; | ||
| if (setIndex == null) { | ||
| break; | ||
| if (intervening) { |
There was a problem hiding this comment.
This function is already enormous. Would it be possible to factor out the new logic into a helper function?
There was a problem hiding this comment.
Done, factored it into isConsumedWithInterveningValue.
f259b19 to
7b0986a
Compare
| } | ||
| auto* binary = inst->origin->dynCast<Binary>(); | ||
| return binary && Properties::isSymmetricInt(binary) && | ||
| getNumConsumedValues(inst) == 2; |
There was a problem hiding this comment.
This line is unneeded: a Binary always consumes 2 values.
There was a problem hiding this comment.
Done, removed the redundant check.
| // Whether a binary operation on integers is symmetric, that is, whether it | ||
| // gives the same result whichever operand order it is computed in. This does | ||
| // not include floating-point operations: swapping their operands can change | ||
| // the NaN bits of the result. |
There was a problem hiding this comment.
Wait, is this actually true for floats? If it is, then OptimizeInstruction's (ancient) call to isSymmetrical is faulty.
There was a problem hiding this comment.
Implementations are allowed but not required to make it so that swapping the NaN operands of a floating point operation could affect the output bits. Since the spec makes no guarantees that depend on the order of the inputs, OptimizeInstructions is fine and we could be more aggressive here.
https://webassembly.github.io/spec/core/exec/numerics.html#nan-propagation
There was a problem hiding this comment.
Sounds good, then let's do this for floats here as well.
There was a problem hiding this comment.
Good catch. Regular Wasm min/max is symmetric here, so I removed isSymmetricInt and use the existing isSymmetric.
| auto* binary = inst->origin->dynCast<Binary>(); | ||
| return binary && Properties::isSymmetricInt(binary) && | ||
| getNumConsumedValues(inst) == 2; | ||
| } |
There was a problem hiding this comment.
Added RefEq and a test for it.
c2d7d24 to
0f9b07b
Compare
| // Whether the local.get at getIndex is consumed together with exactly one | ||
| // intervening value by an operation whose operands are interchangeable, so | ||
| // that removing the set/get pair only swaps them. | ||
| bool StackIROptimizer::isConsumedWithInterveningValue(Index getIndex) { |
There was a problem hiding this comment.
This function doesn't look at the intervening value, though, right? A better name might be isConsumedBySymmetricOp or something like that.
There was a problem hiding this comment.
Good point. Renamed it to isConsumedBySymmetricOp and updated the comment. I also added coverage for a float symmetric op and the two-intervening-values case.
local2Stack gives up when a value sits between a local.set and the matching local.get. If that value and the get are consumed by the same operation whose operands can be swapped, the pair can be removed anyway. This also removes the one size regression of the late optimize-instructions round (4 bytes on a float test module), and is a small win on average.
0f9b07b to
900f480
Compare
artemkulyk
changed the title
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
LGTM! Can you run the fuzzer for a few thousand iterations to make sure it doesn't find any problems? (scripts/fuzz_opt.py or scripts/monitor_fuzz.py -j --max-iters=5000)
|
Ran 10000 fuzz iterations (8 workers, monitor_fuzz.py), no problems found. |
This branch has not been deployed
3 participants
local2Stack gives up when a value sits between a local.set and the matching
local.get. If that value and the get are consumed by the same operation whose
operands can be swapped, the pair can be removed anyway.
This also removes the one size regression of the late optimize-instructions
round (4 bytes on a float test module), and is a small win on average.