|
From: Carl E. L. <ce...@li...> - 2013-07-15 16:17:02
|
On Sun, 2013-07-14 at 23:50 -0500, Peter Bergner wrote:
> On Thu, 2013-07-11 at 07:33 -0700, Carl E. Love wrote:
> > The patch causes the execution flow to take
> > the TM failure path as expected. The compiler is generating a branch if not equal to decide if it
> > should take the TM path or the failure path. For now, the Valgrind patch just assumes the compiler
> > will always generate the branch if not equal instruction to take one of the two paths. Furthermore,
> > it is assumed there will always be a failure path. I need to talk with Peter when he gets back about
> > the code generated by the compiler to determine if the compiler might generate different code
> > sequences.
>
Remember, this patch was for the proposal to just replace the tbegin
with a branch to execute the failure path for the TM. The tbegin and
tend instructions would not actually get executed. Thus we would need
to go find the address for the failure handler and can not get it from
the TM registers. So, your comments below really show the issues with
trying to make this work, i.e. we can't rely on the branch being the
next instruction and we have to handle the compiler generating code
using either beq and bne instructions. The patch is just a proof of
concept patch for 64-bit mode only. I didn't worry about the additional
complexity of handling 32-bit mode as well. But this gives us an idea
of what the issues with the first proposal.
I will keep working on the second proposal from Julian where we do let
the CPU execute the TM instructions then pull the needed failure path
address from the TM register.
> The compiler is free to and actually does generate either a beq or a bne
> depending on the circumstances. Specifically, the code it can generate
> can look like:
>
> tbegin.
> ...
> beq <failure handler>
> // fall-through success handler
>
> or
>
> tbegin.
> ....
> bne <success handler>
> // fall-through failure handler
>
> Note that the "...." above denotes possible instructions that
> might be placed between the tbegin. and the conditional branch,
> so you cannot assume the conditional branch immediately follows
> the tbegin. Luckily, you don't need to look at the branch at
> all. You are only looking at it to compute the address of
> the failure handler, but that is not correct. The address of
> the failure handler is stored in the TFHAR register and the
> tbegin. initializes it to CIA+4 (ie, the next instruction).
>
>
> > + case 0x80: // 128
> > + DIP("mfspr r%u (TFHAR)\n", rD_addr);
> > + putIReg( rD_addr, getGST( PPC_GST_TFHAR) );
> > + break;
> > + case 0x81: // 129
> > + DIP("mfspr r%u (TFIAR)\n", rD_addr);
> > + putIReg( rD_addr, getGST( PPC_GST_TFIAR) );
> > + break;
> > + case 0x82: // 130
> > + DIP("mfspr r%u (TEXASR)\n", rD_addr);
> > + putIReg( rD_addr, getGST( PPC_GST_TEXASR) );
> > + break;
>
> Note that the texasr is a 64-bit register in both 32-bit
> and 64-bit modes. In 32-bit mode, the "mfspr TEXASR,rX"
> should just place the lower 32-bits of the texasr into
> rX. Is that what this code ends up doing?
>
> You are also missing the "mfspr TEXASRU,rX", which is
> used by 32-bit code to get the upper 32-bits of the
> texasr into rX.
>
>
>
> > + case 0x80: // 128
> > + DIP("mtspr r%u (TFHAR)\n", rS_addr);
> > + putGST( PPC_GST_TFHAR, mkexpr(rS) );
> > + break;
> > + case 0x81: // 129
> > + DIP("mtspr r%u (TFIAR)\n", rS_addr);
> > + putGST( PPC_GST_TFIAR, mkexpr(rS) );
> > + break;
> > + case 0x82: // 130
> > + DIP("mtspr r%u (TEXASR)\n", rS_addr);
> > + putGST( PPC_GST_TEXASR, mkexpr(rS) );
> > + break;
>
> Ditto here.
>
>
>
> > + DIP("tbegin. %d\n", R);
> > + if (opc1_next == 0x10) { // conditional branch
> [snip]
>
> As mentional above, there is no need to look at the following
> branch, so your "if (opc1_next == 0x10)" test can be removed.
> Just unconditionally execute the then clause code and remove
> the error code in the else clause.
>
>
> > + /* Get the address of the failure handler from the conditional
> > + * branch in the next instruction location.
> > + */
> > + if ( flag_AA )
> > + failure_tgt = mkSzAddr( ty, extend_s_16to64( BD_u16 ) );
> > + else
> > + failure_tgt = mkSzAddr( ty, guest_CIA_curr_instr +
> > + (Long)extend_s_16to64( BD_u16 ) );
>
> To be pedantic, the address of the failure handler is equal to
> the address that is in the TFHAR register, not the address from
> the conditional branch. That is especially true given that the
> compiler is free to generate either:
>
> tbegin.
> ...
> beq <failure handler>
> // fall-through success handler
>
> or
>
> tbegin.
> ....
> bne <success handler>
> // fall-through failure handler
>
> Remember that tbegin. initializes the TFHAR to CIA+4, so you
> just have to set failure_tgt to the address of nextInstr.
If we execute the tbegin, which is not done according to proposal 1.
>
>
>
> > + /* Set the CR0 field to indicate the tbegin failed. Then let
> > + * the code do the branch to the failure path.
> > + *
> > + * 000 || 0 Transaction initiation successful,
> > + * unnested (Transaction state of
> > + * Non-transactional prior to tbegin.)
> > + * 010 || 0 Transaction initiation successful, nested
> > + * (Transaction state of Transactional
> > + * prior to tbegin.)
> > + * 001 || 0 Transaction initiation unsuccessful,
> > + * (Transaction state of Suspended prior
> > + * to tbegin.)
> > + */
> > + if (mode64)
> > + /* 0x0010 takes transactional path */
> > + /* 0x0000 takes the failure path */
> > + set_CR0(mkU64(0x0000));
> > + else
> > + set_CR0(mkU32(0x0000));
>
> Your comment is correct, but you are incorrectly clearing cr0,
> which signifies the tbegin. succeeded. You need to initialize
> it to 0x2.
Well we are trying to make it execute the failure path, that was the
first proposal, so yes we do need to clear it.
>
> Stylistically, using "0x0000" looks like you're trying to stuff
> 4 4-bit nibbles into cr0, but a cr register can only hold 4-bits
> (ie, 1 nibble). You could use 0x2 or 0b0010, which both look more
> like just 1 nibble of data. Then again, I don't know the valgrind
> code formatting rules and maybe that is how things are written?
> If so, just ignore this comment of mine.
OK, will take the stylistic comments. :-)
>
>
> Peter
>
>
|