xls icon indicating copy to clipboard operation
xls copied to clipboard

Fix divide by constant optimization

Open meheff opened this issue 3 years ago • 1 comments

The divide by constant optimization can crash on some inputs, and sometimes generates bad code.

Crashing repro:

package foo

fn __sample__main(x0: bits[113][8], x2: bits[12], x3: bits[9], x4: bits[5], x5: bits[95], x6: bits[64]) -> bits[64] {
  literal.9: bits[64] = literal(value=65536, id=9, pos=[(0,5,28)])
  ret x9: bits[64] = sdiv(x6, literal.9, id=10, pos=[(0,5,21)])
}

Run with: bazel-bin/xls/tools/opt_main --top __sample__main $IR_FILE

Bad code repro:

package sample

top fn __sample__main(x1: bits[43]) -> bits[43] {
  literal.9: bits[43] = literal(value=1876853526877, id=9, pos=[(0,3,28)])
  ret x6: bits[43] = udiv(x1, literal.9, id=10, pos=[(0,3,21)])
}

Repro:

$ bazel-bin/third_party/xls/tools/eval_ir_main $OPT_IR --input bits[43]:0x555_5555_5555
bits[43]:0x3

Optimize IR with bazel-bin/xls/tools/opt_main and running again yields:

$ blaze-bin/third_party/xls/tools/eval_ir_main $OPT_IR --input bits[43]:0x555_5555_5555
bits[43]:0x2

Also possible to generate a lot of failing samples with the fuzzer. Once the fix is in it'd be good to verify that fuzzing doesn't find any failures. Repro:

$ mkdir /tmp/crasher
$ bazel-bin/xls/fuzzer/run_fuzz_multiprocess  --sample_count=50000  --crash_path=/tmp/crasher

I'll land a change shortly which disables the optimization via a pass constructor argument.

meheff avatar Oct 07 '22 19:10 meheff

My first suspicion, as to the cause of the sometimes-incorrect optimization, is that there may be arithmetic overflow in the functions that compute the constants used (e.g. the multiplicand/reciprocal value). A low-effort fix would be to port the code to use the Bits library instead of int64 data types. This would also solve the issue that this optimization doesn't work for dividends or divisors that are greater than 63 bits. So using the Bits library is something we'd want to try even if this bug didn't exist.

dkillebrew-g avatar Oct 11 '22 15:10 dkillebrew-g