| commit | 7516eec3283d92bb5e353cea60561379312165f4 | |
| author | Mark Williams <mwilliams@fb.com> | |
| Sat, 6 Jan 2018 01:41:18 +0000 (5 17:41 -0800) | ||
| committer | Hhvm Bot <hhvm-bot@users.noreply.github.com> | |
| Sat, 6 Jan 2018 01:55:04 +0000 (5 17:55 -0800) | ||
| tree | 4772ef77ff9e00f767d1453acaae52ef8404721a | treesnapshot (tar.gz zip) |
| parent | 4e0ad79376cce8e2feb504ee9e3f7756c7bfbfab | commitdiff |
Use AssertType in gvn
Summary:
gvn gives up if the SSATmp being replaced is a strict subtype of the
replacement. This is more conservative than necessary, while still
allowing a worse type for the replacement.
The correct check should be that the replacement is a (weak) subtype
of the SSATmp being replaced; and when that check fails, we can simply
insert an AssertType.
At the same time, partially fix a refcounting issue that gvn introduces.
Currently, for instructions that produce a reference, it adds an IncRef for
every use of a replaced SSATmp. At most, it needs one IncRef for every
def of a replaced SSATmp, and this diff makes it do that. This is still
incorrect - there may be paths from the original instruction which need
fewer IncRefs (eg due to side exits, or replacements on mutually exclusive
paths). The result is that we leak such values to the end of the request.
I'll follow up with a proper ref-counting fix.
Reviewed By: swtaarrs
Differential Revision: D6647793
fbshipit-source-id: d8479097d6b595c95ddc5e2e00f8c06002591dd1
Summary:
gvn gives up if the SSATmp being replaced is a strict subtype of the
replacement. This is more conservative than necessary, while still
allowing a worse type for the replacement.
The correct check should be that the replacement is a (weak) subtype
of the SSATmp being replaced; and when that check fails, we can simply
insert an AssertType.
At the same time, partially fix a refcounting issue that gvn introduces.
Currently, for instructions that produce a reference, it adds an IncRef for
every use of a replaced SSATmp. At most, it needs one IncRef for every
def of a replaced SSATmp, and this diff makes it do that. This is still
incorrect - there may be paths from the original instruction which need
fewer IncRefs (eg due to side exits, or replacements on mutually exclusive
paths). The result is that we leak such values to the end of the request.
I'll follow up with a proper ref-counting fix.
Reviewed By: swtaarrs
Differential Revision: D6647793
fbshipit-source-id: d8479097d6b595c95ddc5e2e00f8c06002591dd1
| hphp/runtime/vm/jit/gvn.cpp | diffblobblamehistory |