On 12/26/2013 01:27 PM, Peter Maydell wrote:
>> The only time s->addseg will be false in 16-bit mode is during translation of
>> LEA. I do need the addseg check there for LEA cleanup, but this change
>> should
>> not affect gen_string_movl.
>
> Oh, is that the bit that does:
>
> val = s->addseg;
> s->addseg = 0;
> gen_lea_modrm(env, s, modrm);
> s->addseg = val;
>
> ? I think we should get rid of that -- s->addseg should always
> mean "we can do the segment-base-is-zero optimization",
> it shouldn't be a secret hidden parameter to the gen_lea functions
> saying "don't do this addition".
Perhaps. I'd rather do that as follow-up, since lea_v_seg isn't the top-level
function called for LEA. And if we clean up addseg, we might as well clean up
popl_esp_hack as well.
How about a comment for now?
>> I disagree with the characterization "random temporary". Using the output
>> as a
>> temporary in computing the output is totally sensible, given that our most
>> popular host platform is 2-address.
>
> This is the kind of thing that in an ideal world the register allocator
> would deal with. The tcg/README optimisation suggestions
> don't say anything about preferring to use X,X,Y rather than X,Y,Z
> ops where possible, and typically allocating and using a special
> purpose temp is more readable code.
I agree that a real register allocator would handle it.
I disagree that a special purpose temp would result in more readable code,
since then we'd need to add *more* code to deallocate it after the other
arithmetic.
> More generally, it's pretty unclear to me why we're handling
> "use default segment register for instruction" (ie ovr_seg == -1)
> differently for the three cases.
Because the handling of segments is fundamentally different in each of the
three cpu modes?
> Why is it OK to skip the addition of the base address for ES
> (in the movl_A0_EDI case) when the comment for addseg says
> it only applies to CS/DS/ES?
Err... when are we skipping the addition in gen_string_movl_A0_EDI? We pass in
R_ES as the segment register to use...
> It seems to me that we ought to try to get this code to a
> point where it looks more like:
> if (ovr_seg < 0) {
> ovr_seg = def_seg;
> }
> emit code to get address;
> if (!segment_base_guaranteed_zero(s, ovr_seg)) {
> emit code to add base to address;
> }
>
> where segment_base_guaranteed_zero() is a helper
> function like:
> bool segment_base_guaranteed_zero(s, seg) {
> /* Return true if we can guarantee at translate time that
> * the base address of the specified segment is zero
> * (and thus can skip emitting code to add it)
> */
> return (!s->addseg &&
> (seg == R_CS || seg == R_DS || seg == R_SS));
> }
That does look much nicer, I agree.
r~
