| commit | 8627a7d484032553275c72ca03a09286f4fbbfe2 | |
| author | Shaunak Kishore <kshaunak@fb.com> | |
| Sat, 25 Jan 2020 16:08:37 +0000 (25 08:08 -0800) | ||
| committer | Facebook Github Bot <facebook-github-bot@users.noreply.github.com> | |
| Sat, 25 Jan 2020 16:10:33 +0000 (25 08:10 -0800) | ||
| tree | 9625f4c1b35a091ce5266eefdd0b833815dbb019 | treesnapshot (tar.gz zip) |
| parent | 3f803aa712b6f4851444168a19be339ee15fdc0c | commitdiff |
Re-land stricter type checks for pointer types
Summary:
Original commit changeset: 6748db52e6dc
This is a low-touch version of the original change D19403780. It doesn't introduce full type-tracking for LdLocAddr / LdStkAddr, as the original version did; instead, it simply updates the source types of these ops, makes jit/check.cpp differentiate between "subtypeOf(A,B)" and "subtypeOf(A|B)", and uses FrameState's MBase state to constrain the type of the result of LdMBase instructions.
I thought that this diff would work around the bug, because it was in some component of how we track LdLocAddr / LdStkAddr types. (These types can easily be invalidated by other operations on memory, and FrameState has some handling for it that may not be complete when we use it for more cases.) However, this diff ended up exposing the bugs instead. There are two bugs that I found, and I wrote a small test case to handle them:
- We can convert MemToFrame values into MemToStk values in a DCE pass, and if we do so, we have to retype a bunch of arguments. However, we don't support re-typing the member base register pointer (and indeed, we can't do so, because LdMBase doesn't take a FramePtr src as input).
- By the same token, we don't preserve the type-assertion for LdMBase when we load-elim it away.
The fix I opted for was to drop the type parameter from LdMBase (which isn't being used right now, anyway) and instead add an AssertType afterwards. We can safely assert the type of the pointer's target, and we can even reuse an existing pointer (e.g. one created by LdLocAddr) and infer the memory type that way, because these values will be re-typed in the DCE pass. Load-elim handles updating AssertTypes when their sources change.
Once we confirm this fix, I may up a second diff that adds target-type tracking to LdLocAddr / LdStkAddr / LdMBase as in the original diff.
Reviewed By: ricklavoie
Differential Revision: D19530355
fbshipit-source-id: 2231471e5a69d9fb12e2f0fb91a10f1c52a706d6
Summary:
Original commit changeset: 6748db52e6dc
This is a low-touch version of the original change D19403780. It doesn't introduce full type-tracking for LdLocAddr / LdStkAddr, as the original version did; instead, it simply updates the source types of these ops, makes jit/check.cpp differentiate between "subtypeOf(A,B)" and "subtypeOf(A|B)", and uses FrameState's MBase state to constrain the type of the result of LdMBase instructions.
I thought that this diff would work around the bug, because it was in some component of how we track LdLocAddr / LdStkAddr types. (These types can easily be invalidated by other operations on memory, and FrameState has some handling for it that may not be complete when we use it for more cases.) However, this diff ended up exposing the bugs instead. There are two bugs that I found, and I wrote a small test case to handle them:
- We can convert MemToFrame values into MemToStk values in a DCE pass, and if we do so, we have to retype a bunch of arguments. However, we don't support re-typing the member base register pointer (and indeed, we can't do so, because LdMBase doesn't take a FramePtr src as input).
- By the same token, we don't preserve the type-assertion for LdMBase when we load-elim it away.
The fix I opted for was to drop the type parameter from LdMBase (which isn't being used right now, anyway) and instead add an AssertType afterwards. We can safely assert the type of the pointer's target, and we can even reuse an existing pointer (e.g. one created by LdLocAddr) and infer the memory type that way, because these values will be re-typed in the DCE pass. Load-elim handles updating AssertTypes when their sources change.
Once we confirm this fix, I may up a second diff that adds target-type tracking to LdLocAddr / LdStkAddr / LdMBase as in the original diff.
Reviewed By: ricklavoie
Differential Revision: D19530355
fbshipit-source-id: 2231471e5a69d9fb12e2f0fb91a10f1c52a706d6