Enable ConstraintAnalysis pass - #9010
Conversation
| // in that bad situation where we increment until an integer overflow). | ||
| #ifndef NDEBUG | ||
| static const Index MaxBinaryActions = 5; | ||
| static const Index MaxBinaryActions = 1024 * 1024; |
There was a problem hiding this comment.
Let's fix this by adding widening to the pass rather than by arbitrarily increasing the computation limit.
There was a problem hiding this comment.
"Widening" in what sense? I mean that the pass already has all the widening that I think makes sense to add atm - anything else seems complex and for little benefit, unless you have a good idea?
There was a problem hiding this comment.
We currently have a clever kind of widening that jumps directly to a loop bound. What we don't have is basic widening that goes straight to top when a node is visited too many times. There might be other clever things we could do, but I'm just thinking of the basic pessimistic widening.
There was a problem hiding this comment.
I see, thanks. Ok, I added that after 20 iterations. Meanwhile the fuzzer also found a case that actually needs this, which I added as a test.
| static const Index MaxBinaryActions = 5; | ||
| // propagate them anyhow, so we stop before applying such x = y + 1 | ||
| // operations a ridiculous number of times, by widening to a worst case. | ||
| static const Index MaxBinaryActions = 20; |
There was a problem hiding this comment.
I think this could reasonably be much smaller, e.g. 4.
There was a problem hiding this comment.
I'm not sure. In practice I've seen fuzz testcases with 5. Maybe it's not crucial to handle them, but I didn't look carefully... I would err on the side of not missing out on optimization power here.
2 participants
Example timings measurements:
This does a CFG flow so it does add to compile time. Not as bad as simplify-locals
or precompute-propagate, but slower than most quick passes. And the benefit is
small: less than 1% in all my tests. I do see a benefit on every large testcase I
try, though - including C++, Kotlin, Dart, Java - so this is worth turning on, just not
in -O2 or below, I think.
Also validated on the emscripten test suite, no errors (one code size test will
improve there, by a tiny bit).
Fixes #9001