Re: [Qemu-devel] [PATCH] tcg/arm: Fix double-word comparisons

[Richard Henderson]

On 01/15/2018 06:27 AM, Peter Maydell wrote:
> On 10 January 2018 at 05:39, Richard Henderson <address@hidden> wrote:
>> The code sequence we were generating was only good for unsigned
>> comparisons.  For signed comparisions, use the sequence from gcc.
>>
>> Fixes booting of ppc64 firmware, with a patch changing the code
>> sequence for ppc comparisons.
>>
>> Signed-off-by: Richard Henderson <address@hidden>
>> ---
>>  tcg/arm/tcg-target.inc.c | 112 
>> +++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 80 insertions(+), 32 deletions(-)
>>
>> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
>> index 98a1253..b9890c8 100644
>> --- a/tcg/arm/tcg-target.inc.c
>> +++ b/tcg/arm/tcg-target.inc.c
>> @@ -239,10 +239,11 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int 
>> type,
>>      }
>>  }
>>
>> -#define TCG_CT_CONST_ARM  0x100
>> -#define TCG_CT_CONST_INV  0x200
>> -#define TCG_CT_CONST_NEG  0x400
>> -#define TCG_CT_CONST_ZERO 0x800
>> +#define TCG_CT_CONST_ARM     0x0100
>> +#define TCG_CT_CONST_INV     0x0200
>> +#define TCG_CT_CONST_NEG     0x0400
>> +#define TCG_CT_CONST_INVNEG  0x0800
>> +#define TCG_CT_CONST_ZERO    0x1000
>>
>>  /* parse target specific constraints */
>>  static const char *target_parse_constraint(TCGArgConstraint *ct,
>> @@ -258,6 +259,9 @@ static const char 
>> *target_parse_constraint(TCGArgConstraint *ct,
>>      case 'N': /* The gcc constraint letter is L, already used here.  */
>>          ct->ct |= TCG_CT_CONST_NEG;
>>          break;
>> +    case 'M':
>> +        ct->ct |= TCG_CT_CONST_INVNEG;
>> +        break;
> 
> In GCC constraint "M" is "integer in the range 0 to 32", which this clearly
> isn't. Can we have a comment saying what it is? (may be worth mentioning
> in particular how "rIM" differs from "rINK" -- I think the answer is that
> we want to say "valid-immediate OR (valid-if-inverted AND valid-if-negated)",
> whereas 'rINK' is "valid-immediate OR valid-if-inverted OR valid-if-negated".)

Exactly.

> The code looks OK to me. I'm guessing the sparc guest failure is
> just because we now generate more code for some of the comparison
> cases and that doesn't fit in the buffer...

I don't recall having seen a sparc failure...

> We could avoid the annoying "load LE/GE immediates to tempreg"
> extra code by having tcg_out_cmp2() return a flag to tell
> the caller which way round to put the conditions for its two
> conditional ARITH_MOV insns (for setcond2) or which condition
> to use for the branch (brcond2), right?

Um... we do return a condition.  I must have missed a trick or made a mistake
somewhere.


r~