Primitive return type of loop and try is lost

Question

(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))


Generates the following warnings:


recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b


This is interesting for several reasons.  For one, if the arg to {{recur}} is a {{let}} form, there is no warning:


(fn [] (loop [b 0] (recur (let [a 1] a))))


Also, the compiler appears to understand the return type of {{loop}} forms just fine:


(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}


The problem can of course be worked around using an explicit cast on the {{loop}} form:


(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))


Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31

*See Also:* CLJ-1422

*Patch:* 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch

Answer 1

_Comment made by: a_strangeguy

The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).

so

(fn [] (loop [b 0] (recur (loop [a 1] a))))

gets converted into

(fn [] (loop [b 0] (recur ((fn [] (loop [a 1] a))))))

see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572

this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop

http://dev.clojure.org/jira/browse/CLJ-274

Answer 2

Comment made by: cgrand

loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:

*  type inference doesn't propagate outside of the loop(link: 1) 
*  the return value is never a primitive

• mutable fields are inaccessible
  • surprise allocation of one closure objects each time the loop is entered.

Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.

(link: 1) beware of CLJ-1111 when testing.

Answer 3

Comment made by: alexmiller

I don't think this is going to make it into 1.6, so removing the 1.6 tag.

Answer 4

Comment made by: hiredman

an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn

Answer 5

Comment made by: hiredman

I have attached a 3 part patch as hoistedmethod-pass-1.diff

3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class

9c39cac1 uses HoistedMethod to compile loops not in the return context

901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)

Answer 6

Comment made by: hiredman

with hoistedmethod-pass-1.diff the example code generates bytecode like this

`
user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a))))))
// Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit)
public final class user$eval1675$fn__1676 extends clojure.lang.AFunction {

// Field descriptor #7 Ljava/lang/Object;
public static final java.lang.Object const__0;

// Field descriptor #7 Ljava/lang/Object;
public static final java.lang.Object const__1;

// Method descriptor #10 ()V
// Stack: 2, Locals: 0
public static {};

 0  lconst_0
 1  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
 4  putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18]
 7  lconst_1
 8  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
11  putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20]
14  return
  Line numbers:
    [pc: 0, line: 1]

// Method descriptor #10 ()V
// Stack: 1, Locals: 1
public user$eval1675$fn__1676();

0  aload_0 [this]
1  invokespecial clojure.lang.AFunction() [23]
4  return
  Line numbers:
    [pc: 0, line: 1]

// Method descriptor #25 ()Ljava/lang/Object;
// Stack: 3, Locals: 3
public java.lang.Object invoke();

 0  lconst_0
 1  lstore_1 [b]
 2  aload_0 [this]
 3  lload_1 [b]
 4  invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29]
 7  lstore_1 [b]
 8  goto 2
11  areturn
  Line numbers:
    [pc: 0, line: 1]
  Local variable table:
    [pc: 2, pc: 11] local: b index: 1 type: long
    [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object

// Method descriptor #27 (J)J
// Stack: 2, Locals: 5
public long __hoisted1677(long b);

0  lconst_1
1  lstore_3 [a]
2  lload_3
3  lreturn
  Line numbers:
    [pc: 0, line: 1]
  Local variable table:
    [pc: 2, pc: 3] local: a index: 3 type: long
    [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object
    [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object

}
nil
user=>

`

the body of the method __hoisted1677 is the inner loop

for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version

Answer 7

Comment made by: hiredman

hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr

with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass

Answer 8

Comment made by: alexmiller

Thanks for the work on this!

Answer 9

Comment made by: hiredman

I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do

Answer 10

Comment made by: hiredman

hoistedmethod-pass-3.diff

49782161 add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required

all contribs whose tests pass with master pass with this patch.

the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot

Answer 11

Comment made by: bronsa

Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class

Test case:

user=> (fn [] (try 1 (finally)) 2) VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack user/eval1 (NO_SOURCE_FILE:1)

Answer 12

Comment made by: hiredman

bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too

Answer 13

Comment made by: hiredman

hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code

Answer 14

Comment made by: hiredman

hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff

Answer 15

Comment made by: bronsa

(link: ~hiredman) FWIW the patch no longer applies