|
From: Julian S. <js...@ac...> - 2012-08-23 14:00:16
|
> --- VEX/priv/host_amd64_isel.c (revision 2472)
> +++ VEX/priv/host_amd64_isel.c (working copy)
> @@ -3912,6 +3912,15 @@
> return;
>
> retty = typeOfIRTemp(env->type_env, d->tmp);
> + if (retty == Ity_I128) {
> + HReg dstHi, dstLo;
> + /* The returned value is in %rdx:%rax. Park it in the
> + register-pair associated with tmp. */
> + lookupIRTempPair( &dstHi, &dstLo, env, d->tmp);
> + addInstr(env, mk_iMOVsd_RR(hregAMD64_RDX(),dstHi) );
> + addInstr(env, mk_iMOVsd_RR(hregAMD64_RAX(),dstLo) );
> + return;
> + }
> if (retty == Ity_I64 || retty == Ity_I32
> || retty == Ity_I16 || retty == Ity_I8) {
>
> /* The returned value is in %rax. Park it in the register
>
> This assumes the dirty helper will return an Ity_I128 as a pair of
> 64-bit numbers, consistent with the x86_64 ABI.
>
> So, first question: Does this patch look reasonable?
Yes. I take it that the x86_64 ABI guarantees that all 128 bit
values will be returned in RDX:RAX (yes?) so the above is always correct.
> If so, how would you feel about applying it right away? :-)
I would prefer that we work through some more of the details first,
in order to be sure we have a good design. In particular I am concerned
to arrive at something which also works for 256 bit return values,
and for other architectures.
> Next, once my dirty helper returns its value as an Ity_I128 temporary
> (representing the validity bits for a 128-bit vector load), I need to
> move it into an Ity_V128 temporary (the actual shadow register). It
> looks like I want an "Iop_I128toV128" operand, which does not exist
> (?). But I think I can compose it from Iop_I128to64, Iop_I128HIto64,
> and Iop_64HLtoV128. Does this sound right to you?
That would work, but it is inefficient and burdens the IR generators
with having to insert convoluted IR to work around limitations of a
particular back end. A simpler solution would be to state the
return type of the helper to be Ity_V128 (after all, that is what it is,
essentially) and change the back end to generate code to get the
result value into a 128-bit vector class register:
> + addInstr(env, mk_iMOVsd_RR(hregAMD64_RDX(),dstHi) );
> + addInstr(env, mk_iMOVsd_RR(hregAMD64_RAX(),dstLo) );
then push RAX and RDX on the stack and load into a vector register.
Copy whatever is done for Iop_64HLtoV128. Overall, we should try to
do this better using MOVD et al so as to avoid going via memory, but
I am not sure we can do that and still guarantee to generate SSE2-only
insns. Anyway, a problem for a different day.
> One more question, this time about procedure. In general, would you
> prefer that I propose patches on this mailing list, or that I attach
> them to my bug report?
Patches on bug reports are way better, since those on mailing lists
tend to get lost. The mail list is good for kicking people into
doing reviews, taking notice of patch updates, etc, though.
J
|