RFR: 8322694: C1: Handle Constant and IfOp in NullCheckEliminator [v2]

[Denghui Dong]

On Wed, 7 Feb 2024 00:50:13 GMT, Denghui Dong <ddong at openjdk.org> wrote:

>> src/hotspot/share/c1/c1_Optimizer.cpp line 1218:
>> 
>>> 1216: 
>>> 1217: void NullCheckEliminator::handle_IfOp(IfOp *x) {
>>> 1218:   if (x->type()->is_object() && set_contains(x->tval()) && set_contains(x->fval())) {
>> 
>> What if x->tval() or x->fval() are also IfOp's?  Does NullCheckEliminator visit those depth-first?
>
> Good catch.
> I updated the patch to visit `IfOp` directly, and now it can cover the case where `IfOp`'s `tval` or `fval` are also `IfOp`.
> 
> 
>   // Iterate through block, updating state.
>   for (Instruction* instr = block; instr != nullptr; instr = instr->next()) {
>     // Mark instructions in this block as visitable as they are seen
>     // in the instruction list.  This keeps the iteration from
>     // visiting instructions which are references in other blocks or
>     // visiting instructions more than once.
>     mark_visitable(instr);
>     if (instr->is_pinned() || instr->can_trap() || (instr->as_NullCheck() != nullptr)
>         || (instr->as_Constant() != nullptr && instr->as_Constant()->type()->is_object())
>         || (instr->as_IfOp() != nullptr)) {
>       mark_visited(instr);
>       instr->input_values_do(this);
>       instr->visit(&_visitor);
>     }
>   }

I verified the IR of the following code

  public static int getHash(int i, int j, int k) {
    String text1 = i % 2 == 1 ? "A" : "B";
    String text2 = j % 2 == 1 ? "C" : "D";
    if (k > 1) {
      String text3 = k % 2 == 1 ? text1 : text2;
      return text3.hashCode();
    }
    return 0;
  }



IR after null check elimination
B12 [0, 0] -> B13
empty stack
inlining depth 0
__bci__use__tid____instr____________________________________
. 0    0     50    std entry B13

B13 (S) [0, 0] -> B0 dom B12 pred: B12
empty stack
inlining depth 0
__bci__use__tid____instr____________________________________
. 0    0     49    goto B0

B0 (SV) [0, 33] -> B8 B7 dom B13 pred: B13
empty stack
inlining depth 0
__bci__use__tid____instr____________________________________
  1    0    i15    2
. 2    0    i16    i12 % i15
  3    0    i17    1
  4    0    a51    <instance 0x00007f5024036348 klass=java/lang/String>
  4    0    a52    <instance 0x00007f5024036340 klass=java/lang/String>
  4    0    a53    i16 != i17 ? a51 : a52
. 17   0    i25    i13 % i15
  19   0    a55    <instance 0x00007f5024036358 klass=java/lang/String>
  19   0    a56    <instance 0x00007f5024036350 klass=java/lang/String>
  19   0    a57    i25 != i17 ? a55 : a56
. 33   0     34    if i14 <= i17 then B8 else B7

B7 (V) [36, 56] dom B0 pred: B0
empty stack
inlining depth 0
__bci__use__tid____instr____________________________________
. 38   0    i36    i14 % i15
  40   0    a59    i36 != i17 ? a57 : a53
. 53   0    a42    null_check(a59) (eliminated)
. 53   0    i43    a59.invokespecial()
                   java/lang/String.hashCode()I
. 56   0    i44    ireturn i43

B8 (V) [57, 58] dom B0 pred: B0
empty stack
inlining depth 0
__bci__use__tid____instr____________________________________
  57   0    i45    0
. 58   0    i46    ireturn i45

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17191#discussion_r1480777609