grokking CheckCastNode.canonical()

[Garcia Gutierrez Miguel Alfredo]

Oh, wait. Forget about what I said under (3) below about returning null-node. The way it's done is correct, to preserve the side-effects of the computation denoted by object().

--
Miguel Garcia
Swiss Federal Institute of Technology
EPFL - IC - LAMP1 - INR 328 - Station 14
CH-1015 Lausanne - Switzerland
http://lamp.epfl.ch/~magarcia/

________________________________________
From: graal-dev-bounces at openjdk.java.net [graal-dev-bounces at openjdk.java.net] on behalf of Garcia Gutierrez Miguel Alfredo [miguelalfredo.garcia at epfl.ch]
Sent: Wednesday, July 31, 2013 11:03 PM
To: graal-dev at openjdk.java.net
Subject: grokking CheckCastNode.canonical()

--------------- (1) ---------------

The javadoc for ConstantNode mentions it can represent an "address". Well, no, because a Constant can't represent an address.

/**
 * The {@code ConstantNode} represents a constant such as an integer value, long, float, object
 * reference, address, etc.
 */


--------------- (2) ---------------

In CheckCastNode.canonical() the following seems suspicious:

        // remove checkcast if next node is a more specific checkcast
        if (predecessor() instanceof CheckCastNode) {
            CheckCastNode ccn = (CheckCastNode) predecessor();
            if (ccn != null      &&
                ccn.type != null &&
------------>   ccn == object    &&
                ccn.forStoreCheck == forStoreCheck &&
                ccn.type.isAssignableFrom(type))
            {
                StructuredGraph graph = ccn.graph();
                CheckCastNode newccn = graph.add(new CheckCastNode(type, ccn.object, ccn.profile, ccn.forStoreCheck));
                graph.replaceFixedWithFixed(ccn, newccn);
                return newccn;
            }
        }

IMO, the line with the arrow should read:

                ccn.object == object    &&


--------------- (3) ---------------

I guess it's a matter of coding style, but anyway it would be great to hear other opinions on the following. The context is also CheckCastNode.canonical()

The case where the input is known to be null (reproduced below) is checked in third place, after two more expensive tests. What about making it first?

        if (object().objectStamp().alwaysNull()) {
            return object();
        }

Also in the snippet above, what's the advantage of returning object() vs. returning the uniquified null-node proper, say ConstantNode.forObject(null,...). Is it because of the additional information in the stamp() of object() ?


--
Miguel Garcia
Swiss Federal Institute of Technology
EPFL - IC - LAMP1 - INR 328 - Station 14
CH-1015 Lausanne - Switzerland
http://lamp.epfl.ch/~magarcia/