[9] RFR(S): 8174164: SafePointNode::_replaced_nodes breaks with irreducible loops

[Vladimir Kozlov]

On 2/10/17 1:34 AM, Roland Westrelin wrote:
>
>> Then in what cases your new check (_idx >= idx) fails?
>
> Here is an example:
>
>     static void test_helper(A a) {
>         a.m(); // virtual call with polluted profile
>     }

I assume test_helper() is inlined into test() in all these cases. Right?

>
>     static void inlined() {}
>
>     static A field = new A();
>     static Object test() {
>         A a1 = field;
>         if (a1.getClass() != A.class) { // always fail
>             return null;
>         }
>         A a2 = field;
>         inlined();
>         test_helper(a2);
>         return a1;
>     }
>
>
> Without my change, C2 inlines the a.m() call. The exact class test in
> the test() method for a1 results in a call to replace_in_map that
> replaces the load of field in the map with a CheckCastPP and records
> that pair in the replaced nodes. On return from inlined(), the load of
> field (from a2 = field) is replaced by the CheckCastPP when the replaced
> nodes are processed.

Why we do replacement on exit of method inlined() which not have any code? Should we do it in Call node map before we inline and parse it?

>
> If we drop the inlined() call:
>
>     static Object test() {
>         A a1 = field;
>         if (a1.getClass() != A.class) { // always fail
>             return null;
>         }
>         A a2 = field;
>         test_helper(a2);
>         return a1;
>     }
>
> then C2 doesn't inline the a.m() call anymore. If we put the inlined()
> call back and with the change we're discussing, C2 doesn't inline the
> a.m() call either (because the CheckCastPP is an "old" node on return
> from the inlined call).

Is not this bad (not inlining)?

>
> Again, the replaced nodes were added to propagate casts done in callees
> to callers. This example just happens to optimize well by accident.
>
> The other fix I see would be to disable replaced nodes if one of the
> callers was found to have a irreducible loop by ciTypeFlow. It doesn't
> look much better to me because OSR compilations would be randomly
> affected.

But I think it is smaller number of cases vs current fix. Also most OSR methods are recompiled as normal nmethods so affect will be only temporary.

Thanks,
Vladimir

>
> Roland.
>