|
From: Julian S. <js...@ac...> - 2012-02-07 22:33:34
|
> > I'd suggest that you limit sameIRExpr to recursing only over
> > Un/Bin/Tri/QuadOps, constants and IRTemps, and simply give up
> > on anything else.
>
> Yep. That is what the attached patch does. Regtested on x86_64 and s390x.
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.
+ 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.
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. Still, maybe
we ought to put in a simple recursion depth check that stops it going
nuts in the worst case.
J
|