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

[Vidya Praveen <vidyapraveen at arm dot com>]

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'

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