|
From: Florian K. <br...@ac...> - 2012-02-07 23:13:06
|
On 02/07/2012 05:26 PM, Julian Seward wrote:
>
> Looks good. A couple of comments:
>
> + case Iex_Get:
> + case Iex_Load:
> + /* Guest state / memory could have changed in the meantime. */
> + return False;
> vs
> default:
>
> }
> +
> + return False;
>
> case Iex_GetI also fetches guest state. It will be handled by the
> default case, but to make the commenty bit more complete, could you
> pls add it alongside case Iex_Get and Iex_Load.
>
Yes, good point. Will do.
>
> + case Iex_Const: {
> + IRConst *c1 = e1->Iex.Const.con;
> + IRConst *c2 = e2->Iex.Const.con;
> + vassert(c1->tag == c2->tag);
>
> What makes this assertion true? I think it must be that e1 and e2
> always have the same type. But is that always true? Could we ever
> end up comparing constants of different types? /me thinks not, but
> is not sure.
>
I don't think that assertion can ever be true. Hey, but if you're not
sure, then it should definitely be there :)
> Final q is the effect on performance of the jit, which is already
> way to slow for its own good. Am somewhat concerned about the possibility
> of spending a lot of time in some pathological case, chasing very deep
> expression trees. I guess most trees lead pretty quickly to a Load
> or Get, in which case it stops, so maybe it's not so bad.
When I was debugging this I saw trees with 5 levels and the leaves were
all IRTemps. That does suggest it could get worse.
I want to do some more runs for performance and also to see what this
buys us in terms of saved instructions. Clearly, it won't be earth
shattering, but I'm a still curious...
> Still, maybe
> we ought to put in a simple recursion depth check that stops it going
> nuts in the worst case.
Yeah, we should do that.. I was planning to use a fixed depth across all
iropt levels. If you'd like something more fancy (command line flag to
specify the depth, different settings for different optim levels), let
me know.
Florian
|