[Exp] First prototype of new acmp bytecode

[John Rose]

On Feb 14, 2018, at 7:01 AM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:
> 
> Please let me know if you spot any problems or wrong assumptions. I don't plan to push this code now but just wanted to
> give you an update.

Some comments (on acmp.01):

s/pertub/perturb/ (at least three times)

# templateTable_x86
Suggest `assert((mask >> LogKlassAlignmentInBytes) != 0, "invalid shift")`.
(Same thing in graphKit also.)

Suggest trying memory-to-register cmov instead of test/jcc/bind(is_null)
(or maybe do that in JIT code where it might matter).

# graphKit
Should be structured like this:
    if (ta->is_value_based() || tb->is_value_based()) { ... }
    else if (ta->can be_value_based() && tb->can_be_value_based())) { ... }
    //else use old_acmp

(Looking ahead to subnode, I see you have that there.  There's some
duplication here between the parser and Node::Ideal, probably more
than we want.)

Also in the second case (both *might* be VB) you should swap the operands,
if the second is non-null and the first might be null; that avoids a null check.

In the long term, acmp should have a null_seen profile bit so we can do
trap-based implicit null checks.  I think that would respond to one of your
TODO notes (speculate a being non-null).

The (OrX (CastP2X a) (CastP2X b)) makes my skin crawl a little, but I
guess it's safe, except that I don't think you want the CastX2P wrapped
around it.  That's because the result of the OrX is not a valid oop,
and we don't want it to accidentally be classified as one (say, after
copy removal and LRG merging).

Also, I don't think the OrX will fold up in GVN in all the cases where
it should.  If one operand folds to null, the whole thing should turn
into a null check on the other operand.  If one operand folds to a
NotNull type, the null-test of the whole thing should fold to false.
But that can probably be solved by some pattern matching in
Node::Ideal.  Such extra logic can also turn a CmpX(x,0) to a
CmpP(x,null), when that pops up after GVN.  I think that's one
reason you wanted to add the CastX2P before the CmpP,
instead of a CmpX.

(It's almost as if we want a CmpP(x,y) with a new BoolNode type
of both-zero, where the CmpP/BothZero is lowered to "orcc" or
some such.  And the notion applies to non-pointers also, as an
optimization of things like "if (x == 0 && y == 0)".  The general
case doesn't stop at two inputs, but two inputs *might* be an
interesting case to chase after.)

As with the OrX for double-null test, the output of perturbing
a pointer (OrX with mask) should be an int/long rather than a
pointer.  That is, the CastX2P seems dangerous to me in
both places.  So one of the three inputs of the perturbed
CmpP should be an int/long, not a perturbed oop.

(My problems with CastX2P get less if the result is very
clearly a RawPtr, and has no chance of getting into an
oop map and seen by the GC.)

# parse2

Suggest moving can_be_value_based tests into new_acmp,
where their interaction with is_value_based will be clearer.

# type

TypeInstPtr::can_be_value_based should also test for other supertypes
of value based types, notably interfaces.  For safety, allow all interfaces,
if any value-based types are not final, since a subclass might implement
a new interface.  Speaking of subclasses, you probably want to do an
TypePtr::xmeet rather than klass equality check.

(Thing to remember:  Object functions as an honorary interface to value
types.  So wherever Object is special-cased, interfaces probably belong
in the special case also.)

# subnode

This looks right, except for my reservations about the CastX2P.
(Which might go away if that's a safely marked RawPtr.)

The swapping trick works in subnode.  Specifically:

Node* a; Node *b;
if (might_be_null(a) && !might_be_null(b))
  b = perturb_ptr_if_value(b);
else
  a = perturb_ptr_if_value(a);

Good work!