Re: [Patch][AArch64] - Error on load pair to same register

[Richard Earnshaw <rearnsha at arm dot com>]

On 20/11/14 11:46, Vidya Praveen wrote:
> On Tue, Nov 18, 2014 at 02:01:32PM +0000, Ryan Mansfield wrote:
>> On 14-11-18 05:14 AM, Richard Earnshaw wrote:
>>> On 17/11/14 14:57, Ryan Mansfield wrote:
>>>> 2014-11-17  Ryan Mansfield  <rmansfield@qnx.com>
>>>>
>>>>           * config/tc-aarch64.c (md_assemble): Call warn_unpredictable.
>>>>           (warn_unpredictable): New.
>>>>
>>>> 2014-11-17  Ryan Mansfield  <rmansfield@qnx.com>
>>>>
>>>>           * gas/aarch64/diagnostic.s: Add new warnings test patterns.
>>>>           * gas/aarch64/diagnostic.l: Update expected diagnostic output.
>>>
>>> Ryan,
>>>
>>> Thanks for the patch.
>>>
>>> While I can see merit in handling common cases like loads/stores in one
>>> function, I'm not entirely convinced that having one common function to
>>> handle all unpredictable insns is a good idea - the function could get
>>> very big over time with no real gain to be had from the single
>>> interface.  As such, I'd rename the function to
>>> warn_unpredictable_ldst() or something like that.
>>>
>>> OK with that change.
>>
>> I've updated the function name, and the comment to be specific for 
>> load/stores. If this is OK, would someone please apply the patch on my 
>> behalf? I don't have write privs.
>>
>> Here are the updated changelogs and patch:
>>
>> 2014-11-18  Ryan Mansfield  <rmansfield@qnx.com>
>>
>>          * config/tc-aarch64.c (md_assemble): Call warn_unpredictable_ldst.
>>          (warn_unpredictable_ldst): New.
>>
>> 2014-11-18  Ryan Mansfield  <rmansfield@qnx.com>
>>
>>          * gas/aarch64/diagnostic.s: Add new warnings test patterns.
>>          * gas/aarch64/diagnostic.l: Update expected diagnostic output.
> 
> This seem to cause huge number of gcc test failures in the cases where the
> register numbers are same but not from the same register file. 
> 
> Example:
> 
> /tmp/33604024.0/ccFboXqy.s: Assembler messages:
> /tmp/33604024.0/ccFboXqy.s:355: Warning: unpredictable register after writeback -- `str q0,[x0],16'
> output is:
> /tmp/33604024.0/ccFboXqy.s: Assembler messages:
> /tmp/33604024.0/ccFboXqy.s:355: Warning: unpredictable register after writeback -- `str q0,[x0],16'
> 

Whoops.  Fixed thusly:

2014-11-20  Richard Earnshaw  <rearnsha@arm.com>

	* config/tc-aarch64.c (warn_unpredictable_ldst): Check that transfer
	registers are in the GP register set.  Adjust warnings.  Use correct
	field member for address register.
	* testsuite/gas/aarch64/diagnostic.l: Update.

R.

> How was this tested?
> 
> VP.
> 
> 
>>
>>
>> Regards,
>>
>> Ryan Mansfield
> 
>> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
>> index 8cecfd0..41378f5 100644
>> --- a/gas/config/tc-aarch64.c
>> +++ b/gas/config/tc-aarch64.c
>> @@ -5490,6 +5490,40 @@ programmer_friendly_fixup (aarch64_instruction *instr)
>>    return TRUE;
>>  }
>>  
>> +/* Check for loads and stores that will cause unpredictable behavior */
>> +
>> +static void
>> +warn_unpredictable_ldst (aarch64_instruction *instr, char *str)
>> +{
>> +  aarch64_inst *base = &instr->base;
>> +  const aarch64_opcode *opcode = base->opcode;
>> +  const aarch64_opnd_info *opnds = base->operands;
>> +  switch (opcode->iclass)
>> +    {
>> +    case ldst_pos:
>> +    case ldst_imm9:
>> +    case ldst_unscaled:
>> +    case ldst_unpriv:
>> +      if (opnds[0].reg.regno == opnds[1].reg.regno
>> +	  && opnds[1].addr.writeback)
>> +	as_warn (_("unpredictable register after writeback -- `%s'"), str);
>> +      break;
>> +    case ldstpair_off:
>> +    case ldstnapair_offs:
>> +    case ldstpair_indexed:
>> +      if ((opnds[0].reg.regno == opnds[2].reg.regno
>> +	    || opnds[1].reg.regno == opnds[2].reg.regno)
>> +	  && opnds[2].addr.writeback)
>> +	    as_warn (_("unpredictable register after writeback -- `%s'"), str);
>> +      if ((opcode->opcode & (1 << 22))
>> +	  && opnds[0].reg.regno == opnds[1].reg.regno)
>> +	    as_warn (_("unpredictable load of register pair -- `%s'"), str);
>> +      break;
>> +    default:
>> +      break;
>> +    }
>> +}
>> +
>>  /* A wrapper function to interface with libopcodes on encoding and
>>     record the error message if there is any.
>>  
>> @@ -5622,6 +5656,8 @@ md_assemble (char *str)
>>  	      return;
>>  	    }
>>  
>> +	  warn_unpredictable_ldst (&inst, str);
>> +
>>  	  if (inst.reloc.type == BFD_RELOC_UNUSED
>>  	      || !inst.reloc.need_libopcodes_p)
>>  	    output_inst (NULL);
>> diff --git a/gas/testsuite/gas/aarch64/diagnostic.l b/gas/testsuite/gas/aarch64/diagnostic.l
>> index 322e3c0..d5fdba8 100644
>> --- a/gas/testsuite/gas/aarch64/diagnostic.l
>> +++ b/gas/testsuite/gas/aarch64/diagnostic.l
>> @@ -105,3 +105,12 @@
>>  [^:]*:109: Error: operand 5 should be an integer register -- `sys #0,c0,c0,#0,kk'
>>  [^:]*:110: Error: unexpected comma before the omitted optional operand at operand 5 -- `sys #0,c0,c0,#0,'
>>  [^:]*:112: Error: selected processor does not support `casp w0,w1,w2,w3,\[x4\]'
>> +[^:]*:115: Warning: unpredictable load of register pair -- `ldp x0,x0,\[sp\]'
>> +[^:]*:116: Warning: unpredictable load of register pair -- `ldp d0,d0,\[sp\]'
>> +[^:]*:117: Warning: unpredictable load of register pair -- `ldp x0,x0,\[sp\],#16'
>> +[^:]*:118: Warning: unpredictable load of register pair -- `ldnp x0,x0,\[sp\]'
>> +[^:]*:121: Warning: unpredictable register after writeback -- `ldr x0,\[x0,#8\]!'
>> +[^:]*:122: Warning: unpredictable register after writeback -- `str x0,\[x0,#8\]!'
>> +[^:]*:123: Warning: unpredictable register after writeback -- `str x1,\[x1\],#8'
>> +[^:]*:124: Warning: unpredictable register after writeback -- `stp x0,x1,\[x0,#16\]!'
>> +[^:]*:125: Warning: unpredictable register after writeback -- `ldp x0,x1,\[x1\],#16'
>> diff --git a/gas/testsuite/gas/aarch64/diagnostic.s b/gas/testsuite/gas/aarch64/diagnostic.s
>> index b82ac8b..88001da 100644
>> --- a/gas/testsuite/gas/aarch64/diagnostic.s
>> +++ b/gas/testsuite/gas/aarch64/diagnostic.s
>> @@ -110,3 +110,16 @@
>>  	sys	#0, c0, c0, #0,
>>  
>>  	casp w0,w1,w2,w3,[x4]
>> +
>> +	# test warning of unpredictable load pairs
>> +	ldp     x0, x0, [sp]
>> +	ldp     d0, d0, [sp]
>> +	ldp     x0, x0, [sp], #16
>> +	ldnp    x0, x0, [sp]
>> +
>> +	# test warning of unpredictable writeback
>> +	ldr	x0, [x0, #8]!
>> +	str	x0, [x0, #8]!
>> +	str	x1, [x1], #8
>> +	stp	x0, x1, [x0, #16]!
>> +	ldp	x0, x1, [x1], #16

Attachment: a64-unpred.patch
Description: Text document