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

[Tobias Hartmann]

Hi John,

I've now remembered why I had to create a new node instead of returning a constant in my original
patch. The problem is that we cannot return a (potentially cached) constant from Ideal because
"Idealize should return new nodes, use Identity to return old nodes".

To keep things simple and to avoid code duplication, I've factored the acmp optimizations into
Compile::optimize_acmp which is both used during parsing and during Ideal. We could move the paths
that return constant false into ::Value or ::sub but that would require to duplicate the pattern
matching code and could also not be used during parsing.

I think the best we can do without increasing complexity is this:
http://cr.openjdk.java.net/~thartmann/8205044/webrev.03_inc/

I've added comments explaining why we cannot return a constant. In general, those CmpNodes with
constant input will be immediately folded to constant false by GVN, so although it's a bit hacky,
there is no impact on code generation.

Thanks,
Tobias

On 12.07.2018 19:36, John Rose wrote:
> 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
>