grokking CheckCastNode.canonical()

[Bernhard Urban]

Hi Miguel,

On 07/31/2013 11:03 PM, Garcia Gutierrez Miguel Alfredo wrote:
>
> --------------- (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.
>   */

Well... although we don't have a kind for addresses, we sometimes have 
addresses as constants. For example in HotSpotResolvedObjectType:klass() 
an address as constant with the metaspace pointer is returned, with the 
word kind of the target architecture (which is long on x86_64). Maybe we 
should introduce an address kind.


> --------------- (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    &&

Argh, the comment is wrong, it's actually the other way around, sorry 
about the confusion. The answer is no, because we want to eliminate 
nested checkcasts here, that is we want to look for the pattern "is a 
checkcast an input of another checkcast with a more specific type". For 
example, if you have a method like this:

 > class A1            { long x1; }
 > class A2 extends A1 { long x2; }
 > class A3 extends A2 { long x3; }
 >
 >   public static long test5Snippet(A1 a1) {
 >        long sum = 0;
 >        A2 a2 = (A2) a1;
 >        A3 a3 = (A3) a2;
 >        sum += a2.x2;
 >        return sum + a3.x3;
 >    }

you can get rid of the first checkcast here. Maybe it helps using our 
visualization tool (the example above is actually an unittest) [1]:

$ ./mx.sh igv &
$ ./mx.sh --vm server build
$ ./mx.sh --vm server unittest EliminateNestedCheckCastsTest @-G:Dump= 
@-G:MethodFilter=test5Snippet

in diff mode you see what happens:
http://lafo.ssw.uni-linz.ac.at/lewurm/nestedCheckCasts.png


HTH,
Bernhard


[1]: you have to apply the attached patch, otherwise the debug scopes 
are messed up and MethodFilter wouldn't work.