RFR(M): 8027422: assert(_gvn.type(obj)->higher_equal(tjp)) failed: cast_up is no longer needed (original) (raw)

Roland Westrelin roland.westrelin at oracle.com
Thu Jan 16 01:05:04 PST 2014

Thanks for reviewing this, Christian.

Seeing all these:

true /* includespeculative */ I wonder if we should add new methods for these. It would make it easier to see the users of the speculative versions.

A new meet_speculative() method? I’m ok with it. Vladimir, what do you think?

Roland.

On Jan 14, 2014, at 7:25 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

Looks good to me.

On 1/14/14 1:13 AM, Roland Westrelin wrote: http://cr.openjdk.java.net/~roland/8027422/webrev.02/

I fixed the verification code at the end of Compile::removespeculativetypes(), used this format everywhere: t->filter(type, true /* includespeculative */); renamed NodeHash::checkspeculativetypes() to NodeHash::checknospeculativetypes(), added PhaseIterGVN::checknospeculativetypes() and now call it from the verification code of Compile::removespeculativetypes(). Do I need more than 1 review for this? Yes, you need an other review for this change. Ask Chris or Igor. Thanks, Vladimir

Roland.

On Jan 13, 2014, at 11:03 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote: compile.cpp: new verification code (under ASSERT) at the end of removespeculativetypes() checks only the root node - nothing else is pushed on worklist. Also add comment to this new code. Thanks for catching that. Is: // Verify that after the IGVN is over no speculative type has resurfaced good as a comment? Passing 'true' parameter is not very informative. You can use local variable:

bool includespeculative = true; t->filter(type, includespeculative); An other way to make code more informative is to add a comment to parameter: t->filter(type, true /* includespeculative */); Either way is fine. Will do one of these. Thanks, Roland.

Thanks, Vladimir On 1/10/14 1:46 AM, Roland Westrelin wrote: Hi Vladimir, Here is a new webrev for this: http://cr.openjdk.java.net/~roland/8027422/webrev.01/ I fixed the issues you mentioned in your review. I added a call to removespeculative() to the ConNode constructor. When a node becomes constant, its speculative part can be not null. The IGVN doesn’t kill ConNodes so without a call to removespeculative() a ConNode with a speculative part can sneak past the call to Compile::removespeculativetypes(). I also added a verification method: NodeHash::checkspeculativetypes() to check that no TypeNode with a speculative type is still in the IGVN hash table after Compile::removespeculativetypes() Roland. On Nov 19, 2013, at 8:49 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote: On 11/19/13 11:45 AM, Roland Westrelin wrote: Thanks for reviewing this, Vladimir.

On Nov 19, 2013, at 1:34 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

Next 2 places in type.cpp pass 'true' to meet() unconditionally:

1929 return TypeAry::make(elem->meet(a->elem, true), 3812 const TypeAry *tary = ary->meet(tap->ary, true)->isary(); Should TypeAryPtr::removespeculative() also clean speculative in element's type? You’re right. It probably should. So I need to add a removespeculative() method to TypeAry. Then the 2 places where true is passed to meet() for TypeAry don’t matter anymore because removespeculative() is called from meet() and removespeculative now has an effect on TypeAry, right? Right, passing 'true' will work in all cases then. Thanks, Vladimir

Could you make printing code with 'thist' aligned again in Type::meet()? Sure. Roland.

thanks, Vladimir On 11/18/13 1:15 PM, Roland Westrelin wrote: The root of the problem is that during the null check when the type of obj is improved in GraphKit::castnotnull(): const Type *tnotnull = t->join(TypePtr::NOTNULL, true); The join with TypePtr::NOTNULL is not applied to the speculative part. In fact, no meet between a TypeOopPtr and a TypePtr modifies the speculative part. One way to fix it would be to apply the meet with a TypePtr to the speculative part as well as the standard part of the type which I tried: then we need to move the speculative field up in TypePtr and modify all operations on TypePtr to operate on speculative so that the type system remains symmetric. In many places where we mix a TypePtr with a TypeOopPtr we actually don’t care about the speculative part. I changed the following operations on Type: higherequal() meet() join() filter() so that by default they don’t return a result that include the speculative part of the type. Where we need the speculative part of the type, we have to explicitly request it. I also fixed a problem with Type nodes with a type of TypeNarrowOop that wouldn’t drop the speculative part of the type during Compile::removespeculativetypes(). I included small clean ups that Mikael suggested privately (dropped the duplicate check for res->isaoopptr() in TypeOopPtr::meet, make removespeculative not go through the exercise of creating a new type if speculative is NULL). http://cr.openjdk.java.net/~roland/8027422/webrev.00/ Roland.

More information about the hotspot-compiler-dev mailing list