RFR (S) 8141044: C1 should fold (this == null) to false

[Krystal Mok]

Hi Aleksey,

By the way, your change looks fine to me (not a Reviewer). I like it as-is.

Just for the sake of argument, though, there's an alternative way to
implement the same idea, which could be used as a future enhancement:
instead of tagging the _is_receiver flag on a Local, it could be tagged on
the ObjectType of an HIR instruction. That way this information can exist
in more places, and might even be able to flow across a Phi if all inputs
have (_is_receiver == true).

Let's imagine this kind of example (deriving from your earlier example):

void foo(Bar obj) {
  obj.bar();
  // or if Bar obj came here from a return value of another method
}

void bar() {
  quux(this);
}

void quux(Object o) {
  if (o == null) throw new Exception();
}

If the call path foo() -> bar() -> quux() is fully inlined in C1, then for
the bar() method the 'this' value wouldn't have been a Local[0], thus it'd
miss the optimization you implemented.

But if whenever a value is used as a receiver on some path, after the
NullCheck, tag the type of the value to be _is_receiver = true, then the
example above would be optimized as well.

This alternative is a bit more involved than your original change, since
you can't globally change the type (or any attributes on the type) of a
value if the type only applies to some certain path but not all paths.

For example,

void foo(Bar o, boolean cond) {
  if (cond) {
    o.bar(); // o used as a receiver on this path
  } else {
    // o not used as a receiver on this path
  }
  // Should the type of o be tagged as _is_receiver here? No.
}

So it's better to piggyback on the C1 Optimizer passes to implement this
alternative than in the Canonicalizer.

Just my 2 cents,
- Kris (OpenJDK username: kmo)

On Mon, Nov 2, 2015 at 1:05 PM, Krystal Mok <rednaxelafx at gmail.com> wrote:

> C1's HIR switched to using SSA form in JDK6; before that, the C1 in
> mainstream HotSpot used to use a more traditional expression-tree kind of
> HIR, which also had a "Phi" instruction but not exact in the SSA sense, but
> rather, to serve as a placeholder for values on the expression stack on
> basic block boundaries.
>
> - Kris
>
> On Mon, Nov 2, 2015 at 12:57 PM, John Rose <john.r.rose at oracle.com> wrote:
>
>> On Nov 1, 2015, at 4:59 AM, Aleksey Shipilev <aleksey.shipilev at oracle.com>
>> wrote:
>>
>>
>> Um. I think there is a confusion between "slots" in bytecode and Locals
>> in C1. My cursory reading of C1 code tells me that only the receiver and
>> arguments are exposed as Local-s in C1: see the patch, where only
>> GraphBuilder::state_at_entry creates Locals, and also it is said:
>>
>> // A local is a placeholder for an incoming argument to a function call.
>> LEAF(Local, Instruction)
>>
>> It would seem that we track what instruction had born the value, and
>> Local is a placeholder for "look, it was coming from the outside".
>> Therefore, I think a subsequent astore_0/aload_0 cannot be confused as
>> the receiver.
>>
>>
>> That's right, so my concern is not an issue here.
>>
>> My knowledge of C1 is out of date (or just plain wrong).  I didn't notice
>> they
>> have Phi functions (like C2 does) to fix these sorts of problems.
>>
>> I learn something old every day!
>>
>> — John
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151102/9bb0823b/attachment.html>