Skip to content

The backend never generates iinc #7452

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

Closed
scabug opened this issue May 3, 2013 · 4 comments · Fixed by scala/scala#9713
Closed

The backend never generates iinc #7452

scabug opened this issue May 3, 2013 · 4 comments · Fixed by scala/scala#9713

Comments

@scabug
Copy link

scabug commented May 3, 2013

In the slightly shocking fact of the day dept. It is a decadent waste of bytecode to be generating four instructions including a load and a store when a single instruction can increment in-place.

A typical example - this is a single class:

git diff -w head^ head -- scala/collection/immutable/Range.class
 diff --git a/scala/collection/immutable/Range.class b/scala/collection/immutable/Range.class
 index 6fe5a67ab1..7bf47a582e 100644
 --- a/scala/collection/immutable/Range.class
 +++ b/scala/collection/immutable/Range.class
 @@ -371,10 +371,7 @@ public class scala.collection.immutable.Range extends scala.collection.AbstractS
         : invokestatic  #                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
         : invokeinterface # ,             // InterfaceMethod scala/Function .apply:(Ljava/lang/Object;)Ljava/lang/Object;
         : pop
 -       : iload_2
 -       : iconst_1
 -       : iadd
 -       : istore_2
 +       : iinc           ,
         : iload_3
         : invokevirtual #                 // Method step:()I
         : iadd
 @@ -431,10 +428,7 @@ public class scala.collection.immutable.Range extends scala.collection.AbstractS
         : invokestatic  #                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
         : invokeinterface # ,             // InterfaceMethod scala/Function .apply:(Ljava/lang/Object;)Ljava/lang/Object;
         : pop
 -       : iload
 -       : iconst_1
 -       : iadd
 -       : istore
 +       : iinc           ,
         : iload_3
         : iload
        : iadd
 @@ -529,10 +523,7 @@ public class scala.collection.immutable.Range extends scala.collection.AbstractS
         : iload_2
         : invokeinterface # ,             // InterfaceMethod scala/Function .apply$mcZI$sp:(I)Z
         : ifeq
 -       : iload_3
 -       : iconst_1
 -       : iadd
 -       : istore_3
 +       : iinc           ,
         : iload_2
         : invokevirtual #                 // Method step:()I
         : iadd
 @@ -814,10 +805,7 @@ public class scala.collection.immutable.Range extends scala.collection.AbstractS
         : aload_1
         : iload_3
         : invokeinterface # ,             // InterfaceMethod scala/Function .apply$mcVI$sp:(I)V
 -       : iload
 -       : iconst_1
 -       : iadd
 -       : istore
 +       : iinc           ,
         : iload_3
         : iload
         : iadd
@scabug
Copy link
Author

scabug commented May 3, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7452?orig=1
Reporter: @paulp

@scabug
Copy link
Author

scabug commented May 3, 2013

@paulp said (edited on May 3, 2013 8:26:57 AM UTC):
Unfortunately this is one of those things which ought to be trivial but is made somewhat difficult by the implementation. I guess the easiest thing to do would be a peephole optimization pass right before genasm gets down to emitting bytecode. It will have to have (at least) a four instruction window, looking for sequences like this, where the locals have the same symbol and the second argument to add is constant and fits in a short.

LOAD_LOCAL(variable idx)
CONSTANT(1)
CALL_PRIMITIVE(Arithmetic(ADD,INT))
STORE_LOCAL(variable idx)

Given the local's index and the constant increment, asm has a method to do the rest:

jmethod.visitIincInsn(idx, inc)

@scabug
Copy link
Author

scabug commented May 3, 2013

@magarciaEPFL said:
Fair game for the new backend.

@scabug
Copy link
Author

scabug commented May 3, 2013

@magarciaEPFL said:
Additionally, with GenASM alone it's easier than pre-tweaking ICode: peephole right after genDefDef() has built a MethodNode.

@scabug scabug added the backend label Apr 7, 2017
@scabug scabug added this to the Backlog milestone Apr 7, 2017
harpocrates added a commit to harpocrates/scala that referenced this issue Jul 30, 2021
The backend is now able to turn `x += 42` into an `iinc 42`
instruction. The optimization only applies to `+=` and `-=`, provided
the the net increment fits inside a signed 16-bit value (the ASM library
handles choosing `iinc` or `wide iinc` as is appropriate).

Fixes scala/bug#7452
harpocrates added a commit to harpocrates/scala that referenced this issue Jul 30, 2021
The backend is now able to turn `x += 42` into an `iinc 42`
instruction. The optimization only applies to `+=` and `-=`, provided
the the net increment fits inside a signed 16-bit value (the ASM library
handles choosing `iinc` or `wide iinc` as is appropriate).

Fixes scala/bug#7452
harpocrates added a commit to harpocrates/scala that referenced this issue Jul 30, 2021
The backend is now able to turn `x += 42` into an `iinc 42`
instruction. The optimization only applies to `+=` and `-=`, provided
the the net increment fits inside a signed 16-bit value (the ASM library
handles choosing `iinc` or `wide iinc` as is appropriate).

Fixes scala/bug#7452
harpocrates added a commit to harpocrates/scala that referenced this issue Jul 30, 2021
The backend is now able to turn `x += 42` into an `iinc 42`
instruction. The optimization only applies to `+=` and `-=`, provided
the the net increment fits inside a signed 16-bit value (the ASM library
handles choosing `iinc` or `wide iinc` as is appropriate).

Fixes scala/bug#7452
@SethTisue SethTisue modified the milestones: Backlog, 2.13.7 Jul 30, 2021
harpocrates added a commit to harpocrates/scala that referenced this issue Jul 30, 2021
The backend is now able to turn `x += 42` into an `iinc 42`
instruction. The optimization only applies to `+=` and `-=`, provided
the the net increment fits inside a signed 16-bit value (the ASM library
handles choosing `iinc` or `wide iinc` as is appropriate).

Fixes scala/bug#7452
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants