RFR(M): 8073480: C2 should optimize explicit range checks

[Roland Westrelin]

Thanks for the review, Vladimir.

Roland.

> On Mar 17, 2015, at 6:37 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> This looks good.
> 
> Thanks,
> Vladimir
> 
> On 3/17/15 2:19 AM, Roland Westrelin wrote:
>> Thanks, Vladimir.
>> 
>> Here is a new webrev:
>> 
>> http://cr.openjdk.java.net/~roland/8073480/webrev.02/
>> 
>> With the non-static methods in BoolTest, a search up the dominator tree in improve_address_types() to handle the unsafe volatile issue that Paul spotted and fixed reroute_side_effect_free_unc() that doesn’t ignore return values from igvn->transform().
>> 
>> Roland.
>> 
>> 
>>> On Mar 13, 2015, at 6:52 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>> 
>>> 
>>> 
>>> On 3/13/15 6:39 AM, Roland Westrelin wrote:
>>>> Thanks for reviewing this Vladimir.
>>>> 
>>>>> In general this looks good.
>>>>> 
>>>>> There are a lot of BoolTest:: checks. May be we should add new static methods to BoolTest class.
>>>> 
>>>> What tests do you think should go in separate methods?
>>>> 
>>>> Simple tests like this:
>>>> bn->_test._test == BoolTest::le
>>>> 
>>>> Or complex tests like:
>>>>     in(1)->as_Bool()->_test._test != BoolTest::ne &&
>>>>     in(1)->as_Bool()->_test._test != BoolTest::eq &&
>>>>     in(1)->as_Bool()->_test._test != BoolTest::overflow &&
>>>>     in(1)->as_Bool()->_test._test != BoolTest::no_overflow;
>>>> 
>>>> ?
>>> 
>>> non-static methods in BoolTest:
>>> 
>>>  bool is_less_or_equal( )  const { return _test == BoolTest::lt || _test == BoolTest::le; }
>>>  bool is_great_or_equal( ) const { return _test == BoolTest::gt || _test == BoolTest::ge; }
>>> 
>>> The above checks will be:
>>> 
>>> (in(1)->as_Bool()->_test.is_less_or_equal() ||
>>> in(1)->as_Bool()->_test.is_great_or_equal()) &&
>>> 
>>> And they can be used in asserts in fold_compares_helper().
>>> 
>>>> 
>>>>> I see you use in several places (mostly in reroute_side_effect_free_unc()) igvn->transform() without overwriting initial node:
>>>>> 
>>>>> igvn->transform(use);
>>>>> 
>>>>> which may not be correct if for some reasons node is transformed. You need to do:
>>>>> 
>>>>> use = igvn->transform(use);
>>>> 
>>>> Right. Thanks for spotting that. But in the case of reroute_side_effect_free_unc, the nodes are connected together and then transformed so I should only need:
>>>> 
>>>> halt = igvn->transform(halt);
>>>> 
>>>> right?
>>> 
>>> I think you need to do transform after set_req:
>>> 
>>> +   Node* c = otherproj->clone();
>>> +   new_unc->set_req(TypeFunc::Parms, unc->in(TypeFunc::Parms));
>>> +   new_unc->set_req(0, c);
>>> +   new_unc = igvn->transform(new_unc);
>>> +   call_proj->set_req(0, new_unc);
>>> +   call_proj = igvn->transform(call_proj);
>>> +   halt->set_req(0, call_proj);
>>> +   halt = igvn->transform(halt);
>>> +
>>> +   igvn->C->root()->add_req(halt);
>>> +   igvn->replace_node(otherproj, igvn->C->top());
>>> 
>>> 
>>>> 
>>>>> reroute_side_effect_free_unc() why clone uncommon trap and not use new Region node? You can pass flag to merge_uncommon_traps() ot indicate when Region node is available already.
>>>> 
>>>> reroute_side_effect_free_unc changes the type of the trap: it takes the unc call from the first if (most likely an Reason_unstable_if trap) and sets the reason to the reason of the unc for the middle if which, given the middle check is now restricted to a null check, should be Reason_null_check. Thinking more about this, I don’t think it’s required that we keep the null check reason for the middle unc for that machinery to work as expected but maybe keeping the null check can help when trying to diagnose why we hit a unc here?
>>> 
>>> You can try to create Phi node with different reasons if you want to distinguish reasons but have only one unc trap. But it will eat register on fastpath so it is not good idea. Okay keep it as you said to see null check reason.
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> 
>>>> Roland.
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 3/12/15 10:34 AM, Roland Westrelin wrote:
>>>>>> Here is a new webrev for this:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~roland/8073480/webrev.01/
>>>>>> 
>>>>>> I took Vladimir’s comments into account (added test for null inputs in several places, strengthen the test to make sure a middle guard is a null check, renamed functions) and added code that look for a ConvI2L between the range check and a memory access that follows and annotate that ConvI2L with a tighter type so the movslq that Paul spotted are removed from the final code.
>>>>>> 
>>>>>> Roland.
>>