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

Roland Westrelin roland.westrelin at oracle.com
Tue Mar 17 09:19:22 UTC 2015

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)->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 islessorequal( ) const { return test == BoolTest::lt || test == BoolTest::le; } bool isgreatorequal( ) const { return test == BoolTest::gt || test == BoolTest::ge; } The above checks will be: (in(1)->asBool()->test.islessorequal() || in(1)->asBool()->test.isgreatorequal()) && And they can be used in asserts in foldcompareshelper(). 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 setreq: + Node* c = otherproj->clone(); + newunc->setreq(TypeFunc::Parms, unc->in(TypeFunc::Parms)); + newunc->setreq(0, c); + newunc = igvn->transform(newunc); + callproj->setreq(0, newunc); + callproj = igvn->transform(callproj); + halt->setreq(0, callproj); + halt = igvn->transform(halt); + + igvn->C->root()->addreq(halt); + igvn->replacenode(otherproj, igvn->C->top());

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