RFR(L): 8205044: [lworld] Interpreter and compiler support for acmp with value type operands
john.r.rose at oracle.com
Thu Jul 12 17:36:47 UTC 2018
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.
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?
> 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!
More information about the valhalla-dev