RFR(L): 8205044: [lworld] Interpreter and compiler support for acmp with value type operands

[John Rose]

Yes, that's much better.

The type of phase->longcon(1) is wrong; you mean intcon(1).

Also, even intcon(1) is confusing; I think we want something that
is more clearly related to the TypeInt::CC constants.  In this case
TypeInt::CC_GT is exactly what you want, since it asserts that
the variable value is greater than zero, and unsigned pointer
comparison is a reasonable fiction here.  So, you can say
makecon(TypeInt::CC_GT).  There is no CC_NE, for various
reasons.  You could polish it up by adding TypeInt::CC_NOT_NULL
as an alias for CC_GT.

— John

On Jul 12, 2018, at 1:32 AM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:
> 
> Hi John,
> 
> thanks for looking at this!
> 
> On 12.07.2018 09:35, John Rose wrote:
>> I was liking it all the way to the last file, especially the
>> part that went "speculate, speculate, speculate"–truly
>> a sentiment for our times.  When I got to the last file,
>> with CmpLNode::Ideal, I noticed that it seems to maybe
>> work correctly for the particular graph shapes created
>> elsewhere, but not considered in isolation.  Note that
>> there is no reference to in(2), only in(1)->in([1 or 2]).
>> It can't be correct, in general, to execute the non-null
>> returns, if you didn't inspect in(2).  I think you need
>> more pattern-matching here; it's too eager to get to
>> the win as coded.
> 
> Right, that code relies on the fact that we currently don't have a CmpLNode with a
> OrLNode(CastP2XNode, CastP2XNode) input ever. That code pattern is only generated by
> Compile::optimize_acmp(). But I agree that we should strengthen the CmpLNode::Ideal() pattern
> matching by looking at in(2) as well.
> 
> What about this?
> http://cr.openjdk.java.net/~thartmann/8205044/webrev.02_inc/
> 
> I've already pushed (right before receiving your email) but will push this as a follow up fix.
> 
>> I reviewed it and approve (heartily) of all of it, except
>> for the above misgiving.
> 
> Thank you!
> 
> Tobias