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

Vladimir Kozlov vladimir.kozlov at oracle.com
Fri Mar 13 17:52:36 UTC 2015

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)->asBool()->test.test != BoolTest::ne && in(1)->asBool()->test.test != BoolTest::eq && in(1)->asBool()->test.test != BoolTest::overflow && in(1)->asBool()->test.test != BoolTest::nooverflow; ?

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 reroutesideeffectfreeunc()) 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 reroutesideeffectfreeunc, 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:

reroutesideeffectfreeunc() why clone uncommon trap and not use new Region node? You can pass flag to mergeuncommontraps() ot indicate when Region node is available already. reroutesideeffectfreeunc changes the type of the trap: it takes the unc call from the first if (most likely an Reasonunstableif 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 Reasonnullcheck. 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.

More information about the hotspot-compiler-dev mailing list