Hi Tomek,
On Wed, 2009-04-22 at 23:09 +0200, Tomek Grabiec wrote:
> Explanation: Control is transferred between basic blocks not only by branch
> instructions such as goto/if*/athrow but it can be transferred between
> adjacent basic blocks implicitly - after the last instruction of some basic
> block has been executed, the control enters the following basic block. The
> simplest way to handle this case is to emit native code such that basic blocks
> are in the same order as they are in bytecode. In that way, if there was
> implicit control transfer between basic blocks in bytecode there will also be
> one in jitted code. This is achieved by sorting basic blocks by the 'start'
> counter before emission and after all basic blocks are generated.
>
> One of the impacts of this fix is correct compilation of for loops:
>
> public static void testForLoop() {
> int i;
> for (i = 0; i < 10; i++)
>
> assertEquals(i, 10);
> }
>
> Signed-off-by: Tomek Grabiec <[email protected]>
Applied, thanks!
I fixed up some issues by hand so I'll just go through them here so I
don't have to do that all the time. Please double-check the result in
master.
> +/**
> + * list_sort - sort the list using given comparator
> + * @head: is a head of the list to be sorted
> + * @comaprator: function comparing two entries, should return value lesser
> + * than 0 when the first argument is lesser than the second one.
> + */
> +void list_sort(struct list_head *head,
> + int (*comparator)(const void *, const void *));
This is pretty bad from API point of view. I fixed up "comparator" to
take "struct list_head **" type arguments.
> @@ -165,3 +165,18 @@ void compute_insn_positions(struct compilation_unit *cu)
> }
> }
>
> +static int bb_comparator(const struct list_head **e1,
> + const struct list_head **e2)
> +{
> + struct basic_block *bb1, *bb2;
> +
> + bb1 = list_entry(*e1,struct basic_block,bb_list_node);
> + bb2 = list_entry(*e2,struct basic_block,bb_list_node);
There was some whitespace missing here.
> +void sort_basic_blocks(struct compilation_unit *cu)
> +{
> + list_sort(&cu->bb_list,bb_comparator);
And here.
> diff --git a/vm/list.c b/vm/list.c
> new file mode 100644
> index 0000000..40f3a84
> --- /dev/null
> +++ b/vm/list.c
> @@ -0,0 +1,62 @@
> +/*
> + * Copyright (c) 2009 Tomasz Grabiec
> + *
> + * This file is released under the GPL version 2 with the following
> + * clarification and special exception:
> + *
> + * Linking this library statically or dynamically with other modules is
> + * making a combined work based on this library. Thus, the terms and
> + * conditions of the GNU General Public License cover the whole
> + * combination.
> + *
> + * As a special exception, the copyright holders of this library give you
> + * permission to link this library with independent modules to produce an
> + * executable, regardless of the license terms of these independent
> + * modules, and to copy and distribute the resulting executable under
> terms
> + * of your choice, provided that you also meet, for each linked
> independent
> + * module, the terms and conditions of the license of that module. An
> + * independent module is a module which is not derived from or based on
> + * this library. If you modify this library, you may extend this
> exception
> + * to your version of the library, but you are not obligated to do so. If
> + * you do not wish to do so, delete this exception statement from your
> + * version.
> + *
> + * Please refer to the file LICENSE for details.
> + */
> +
> +#include <vm/list.h>
> +#include <stdio.h>
This didn't compile. For realloc(), you need:
#include <stdlib.h>
> +void list_sort(struct list_head *head,
> + int (*comparator)(const void *, const void *))
I introduced a typedef for the comparison function.
> +{
> + int len;
> + int i;
> + struct list_head **arr;
> + struct list_head *node;
> +
> + len = 0;
> + arr = 0;
0 != NULL
Please use NULL for pointers. I wonder if we should hook up sparse to go
through Jato code...
> + list_for_each(node, head) {
> + struct list_head **new_arr;
> +
> + new_arr = realloc(arr,sizeof(struct list_head*) * (len+1));
> + if (!new_arr) {
> + fprintf(stderr,"%s:out of memory.\n",__FUNCTION__);
> + if (arr) free(arr);
> + return;
This is wrong. Either you die() or you handle the error. I did the
latter.
> + }
> +
> + arr = new_arr;
> + arr[len++] = node;
> + }
> +
> + qsort(arr, len, sizeof(struct list_head*), comparator);
Some whitespace missing before the asterisk of the third argument.
> +
> + INIT_LIST_HEAD(head);
> +
> + for (i = 0; i < len; i++)
> + list_add_tail(arr[i], head);
> +
> + free(arr);
> +}
Furthermore, this patch broke "make test" which I also fixed up.
Overall, please do bit more QA before submitting a patch. It wasn't a
huge deal for this one (especially as it's such an important fix!) but I
obviously won't be able to do this for all patches.
Pekka
------------------------------------------------------------------------------
Stay on top of everything new and different, both inside and
around Java (TM) technology - register by April 22, and save
$200 on the JavaOne (SM) conference, June 2-5, 2009, San Francisco.
300 plus technical and hands-on sessions. Register today.
Use priority code J9JMT32. http://p.sf.net/sfu/p
_______________________________________________
Jatovm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/jatovm-devel
