binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

[OptimizeInstructions] Invert sign for negative RHS constants in signed rem

Open MaxGraey opened this issue 2 years ago • 8 comments

i32(x) % -C   =>   i32(x) % C
i64(x) % -C   =>   i64(x) % C,   if  C != C_min

It may reduce size (however not sure) and canonicalize expression for further optimizations like (signed)x % C_pot != 0 and remove special (signed)x % -1 ==> 0 rule.

MaxGraey avatar Sep 04 '21 11:09 MaxGraey

Fuzzed: ITERATION: 11165

MaxGraey avatar Sep 04 '21 13:09 MaxGraey

This looks correct, but I wonder if we should look into size some more. This will increase size in some cases as negative numbers can be smaller due to how LEBs work (see here). OTOH positive numbers are more common, so I'd expect gzip to prefer positive ones. As rem is rare, this may not matter much, though.

kripken avatar Sep 07 '21 21:09 kripken

Yes, negative denonimators quite uncommon especially those who might have benefited from negative value. Besides, it would make the logic very complicated if we take this into account

MaxGraey avatar Sep 10 '21 15:09 MaxGraey

@kripken Can we merge it?

MaxGraey avatar Sep 14 '21 20:09 MaxGraey

@tlively @kripken Could you merge it while it already approved?

MaxGraey avatar Aug 27 '22 08:08 MaxGraey

I would be happy to merge, but I want to make sure @kripken is ok with the size question first.

tlively avatar Aug 27 '22 20:08 tlively

I think if we do some measurements on this that would be enough. A few large real-world wasm files would be enough. I can help find some if you need that @MaxGraey

kripken avatar Sep 13 '22 23:09 kripken

A few large real-world wasm files would be enough. I can help find some if you need that

It should be non-LLVM binaryen due to LLVM already do this canonization

MaxGraey avatar Sep 14 '22 08:09 MaxGraey