Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

box/unbox optimization confounded by branch #11779

Open
hrhino opened this issue Oct 26, 2019 · 4 comments

Comments

@hrhino
Copy link
Member

@hrhino hrhino commented Oct 26, 2019

With -opt:l:method:

scala> def test1(i: Int, j: Int) = { val (w, t) = (i + j, i - j) ; w + t }
test1: (i: Int, j: Int)Int

scala> :javap test1
  public int test1(int, int);
    descriptor: (II)I
    flags: ACC_PUBLIC
    Code:
      stack=3, locals=3, args_size=3
         0: iload_1
         1: iload_2
         2: iadd
         3: iload_1
         4: iload_2
         5: isub
         6: iadd
         7: ireturn
      LineNumberTable:
        line 11: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       8     0  this   L$line3/$read$$iw$$iw$;
            0       8     1     i   I
            0       8     2     j   I

but

scala> def test2(i: Int, j: Int) = { val (w, t) = { if (i > j) (i, j) else (j, i) }; w + t }
test2: (i: Int, j: Int)Int

scala> :javap test1
  public int test2(int, int);
    descriptor: (II)I
    flags: ACC_PUBLIC
    Code:
      stack=4, locals=6, args_size=3
         0: iload_1
         1: iload_2
         2: if_icmple     17
         5: new           #23                 // class scala/Tuple2$mcII$sp
         8: dup
         9: iload_1
        10: iload_2
        11: invokespecial #26                 // Method scala/Tuple2$mcII$sp."<init>":(II)V
        14: goto          26
        17: new           #23                 // class scala/Tuple2$mcII$sp
        20: dup
        21: iload_2
        22: iload_1
        23: invokespecial #26                 // Method scala/Tuple2$mcII$sp."<init>":(II)V
        26: astore_3
        27: aload_3
        28: invokevirtual #32                 // Method scala/Tuple2._1$mcI$sp:()I
        31: istore        4
        33: aload_3
        34: invokevirtual #35                 // Method scala/Tuple2._2$mcI$sp:()I
        37: istore        5
        39: iload         4
        41: iload         5
        43: iadd
        44: ireturn
      StackMapTable: number_of_entries = 2
        frame_type = 17 /* same */
        frame_type = 72 /* same_locals_1_stack_item */
          stack = [ class scala/Tuple2$mcII$sp ]
      LineNumberTable:
        line 11: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
           31       8     4     w   I
           37       2     5     t   I
            0      45     0  this   L$line4/$read$$iw$$iw$;
            0      45     1     i   I
            0      45     2     j   I

I expect that the check in checkInstanceCreation is slightly too strict: in the latter case, #26 is a consumer of both #8 and #20, and (if I understand the box/unbox code properly) this should be able to be handled by M2. Arguably it shouldn't matter who's consuming the value on stack after the INVOKESPECIAL, right?

@hrhino hrhino added this to the Backlog milestone Oct 26, 2019
@som-snytt

This comment has been minimized.

Copy link

@som-snytt som-snytt commented Oct 26, 2019

Indeed, it looks like REPL has entirely rewritten your test1 method as test2. I wonder if there is already a ticket for that.

@hrhino

This comment has been minimized.

Copy link
Member Author

@hrhino hrhino commented Oct 26, 2019

Ah shoot, how did that happen? I'll fix it when I get home. Can you add the repl label please?

@lrytz

This comment has been minimized.

Copy link
Member

@lrytz lrytz commented Oct 29, 2019

@hrhino I took a look, here's a draft: scala/scala@2.13.x...lrytz:t11779

cfr then shows

public class C {
    public int test2(int i, int j) {
        int n;
        int n2;
        if (i > j) {
            n = i;
            n2 = j;
        } else {
            n = j;
            n2 = i;
        }
        return n + n2;
    }
}

Let me know if you're interested in finishing it up. Maybe we have the same problem and need a Drop consumer for other box kinds than tuples?

@hrhino

This comment has been minimized.

Copy link
Member Author

@hrhino hrhino commented Oct 29, 2019

Thanks! I'll gladly poke at it this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.