On 05/17/2011 01:19 AM, Ulrich Weigand wrote:
> Yao Qi wrote:
>>
>>      POP {r0, r1, ...., r6};
>>      POP {r7};
> 
> The above can use just a single POP {r0, ..., r7}, can't it?

Yes, we can.  Why didn't I combine these two instructions then?

> Have you looked at how the ARM case does it?  There, we still have just
> a single POP { r0, ..., rN } that pops the right number of registers,
> and then the cleanup function (cleanup_block_load_pc) reshuffles them.
> It seems to me we could do the same (and actually use the same cleanup
> function) for the Thumb case too ...

Sure, we can reuse that for Thumb case here.  In this case, when
register list is not full, we could optimize it a little bit like what I
did in my last patch.  However, it is a separate issue, and can be
addressed separately.

>> 3.  register list is empty.  This case is relative simple.
>>
>>      POP {r0}
>>
>> In cleanup, we store r0's value to PC.
> 
> If we used cleanup_block_load_pc, this would handle the same case as well.
> 
> (Unfortunately, handling case 1 the same way looks somewhat difficult,
> since cleanup_block_load_pc would expect the PC in register r8 ...)
> 

In my new patch, there are two different cases to handle POP instruction.
1.  register list is full.  Use the following code sequence,

     POP {r0, r1, ...., r6, r7}; remove PC from reglist
     MOV r8, r7; Move value of r7 to r8;
     POP {r7}; Store PC value into r7.

Install cleanup routine cleanup_pop_pc_16bit_all (renamed from
cleanup_pop_pc_16bit)

2.  register list is not full.  Similar to arm part (arm_copy_block_xfer)

>> +cleanup_pop_pc_16bit(struct gdbarch *gdbarch, struct regcache *regs,
>> +		     struct displaced_step_closure *dsc)
> 
> One more space before ( ...
> 

Sorry about that.  Fixed.

>> +  else /* Cleanup procedure of case #2 and case #3 can be unified.  */
>> +    {
>> +      int rx = 0;
>> +      int rx_val = 0;
>> +
>> +      if (dsc->u.block.regmask)
>> +	{
>> +	  for (rx = 0; rx < 8; rx++)
>> +	    if ((dsc->u.block.regmask & (1 << rx)) == 0)
>> +	      break;
>> +	}
>> +      else
>> +	rx = 0;
> 
> (This is irrelevant if we decide to use cleanup_block_load_pc, but:
> the "if (dsc->u.block.regmask)" and "else rx = 0" are superfluous,
> since the for loop will terminate with rx == 0 anyway if regmask
> is zero.)

This part is removed since cleanup_block_load_pc is used.

-- 
Yao (齐尧)