Repository navigation
Conversation
|
This turned out more complex than I had anticipated. Conceptually this mimics a part of type inference again, and seems fragile. There must be a nicer way to do this. |
dcf148c to
b0f610f
Compare
|
A comparison bound says nothing about the int value of a bool, string or array ( Also fixed assignment through a $GLOBALS typed reference in b0f610f. |
|
I'm not reviewing ai generated code, it tends to bolt things on top rather than doing an integrated fix, and it definitely feels like this is the case here |
|
There is a fair bit of human slop (mine) here too (I dunno if that makes it worse 🥹) As a general note, in case you or someone else wants to pursue different path. This change gives smallest performance impact (although it is still 1-2% in micro bench) other approaches are fair more painful 5% impact, although they may "look cleaner or general" (had LLM draft variants for review), but I didn't like them mostly for performance reasons, and actually scope started to seem more dangerous even to me. |
|
The globals issue seems SCCP related rather than type inference related. It reminds me of a patch (in an open PR) I have for typed references/properties. |
I really wish to know how much is automatic and how much is done by hand. You open PRs at record speeds, sometimes in such a short span that it must be automatically opened. Obviously your patches are largely automated. On first thought on this patch: I think it's cleaner to rerun range inference+type inference until a fixpoint is reached. Dropping the ranges on constrained inputs after type inference based on a loop over the phi nodes. Sure there's a hit, but I expect this to be rare. |
|
Not sure if it is actually SCCP related, might be worth a second look. |
|
To 1st part replied privately ;-) On to the patch itself, some meat tokens later... Tried the fix point idea, it fixes the Pi cases and keeps thing to a long while this pr widens it. Performance is not great based on run-tests.php it reruns 7 of 102 functions, and those hold 40% of the opcodes, so it costs +4.8% instructions in opcache_compile_file() vs +1.9% for this PR (callgrind, debug build). A chained case also needs more than one rerun. 4.8 vs 1.9 seems like a no-go to me.
|
Yuck
No, that deserves its own PR, shouldn't get folded. |
Range inference runs before type inference, so a Pi constraint such as $v > 5 && $v < 7 narrows the range of a value that may be a float, bool or string. That range only describes the integer values, but casts, arithmetic and bitwise operations on other types, symbolic bounds, call results and division by zero checks read it as a bound on the converted value. After inference, check those consumers against the final types, and only when one reads a range derived from such a constraint, drop the offending constraints and rerun range and type inference. Fixes phpGH-24088
b0f610f to
c0149f2
Compare
|
OCD is a thing.. Rerun now only happens when a range from a Pi on a possibly non-int value reaches an int conversion or a symbolic bound, then those Pis are dropped and inference rerun until clean. It doesn't fire on run-tests.php and costs +0.3% instructions there, vs +4.8% rerunning unconditionally. Globals bit moved to its own PR. |
2 participants
Range inference runs before type inference, so a comparison such as
$v > 5 && $v < 7gives the Pi node a range of [6..6] even when $v may be 5.5 or true. That range is only valid for the integer values, but casts, arithmetic on non-integer operands, symbolic bounds, call results and the division by zero check read it as a bound on the converted value, so SCCP folds (int) 5.5 to 6. After inference, the consumers that convert to integer are checked against the final types; only if one of them reads a range derived from such a Pi are those Pis dropped and inference rerun. This keeps the existing range precision and costs about 0.3% instructions in opcache_compile_file() on run-tests.php. Fixes #24088.