[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-graal-dev
Subject:    RE: grokking CheckCastNode.canonical()
From:       Garcia Gutierrez Miguel Alfredo <miguelalfredo.garcia () epfl ! ch>
Date:       2013-07-31 22:07:05
Message-ID: 7E4228B446372948BBB2916FC53FA49E26DE475F () REXMD ! intranet ! epfl ! ch
[Download RAW message or body]


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@openjdk.java.net [graal-dev-bounces@openjdk.java.net] on \
                behalf of Garcia Gutierrez Miguel Alfredo \
                [miguelalfredo.garcia@epfl.ch]
Sent: Wednesday, July 31, 2013 11:03 PM
To: graal-dev@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/
=


[prev in list] [next in list] [prev in thread] [next in thread]