Re: [Patch][Aarch64] Refactor comments in aarch64_print_operand

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Tue, Jul 11, 2017 at 05:29:11PM +0100, Jackson Woodruff wrote:
> Hi all,
> 
> This patch refactors comments in config/aarch64/aarch64.c
> aarch64_print_operand
> to provide a table of aarch64 specific formating options.
> 
> I've tested the patch with a bootstrap and testsuite run on aarch64.
> 
> OK for trunk?

Hi Jackson,

Thanks for the patch, I have a few comments, but overall this looks
like a nice improvement.

> Changelog:
> 
> gcc/
> 
> 2017-07-04  Jackson Woodruff  <jackson.woodruff@arm.com>
> 
>      * config/aarch64/aarch64.c (aarch64_print_operand):
>        Move comments to top of function.

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 037339d431d80c49699446e548d6b2707883b6a8..91bf4b3e9792e4ba01232f099ed844bdf23392fa 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -5053,12 +5053,39 @@ static const int aarch64_nzcv_codes[] =
>    0		/* NV, Any.  */
>  };
>  
> +/* aarch64 specific string formatting commands:

s/aarch64/AArch64/
s/string/operand/

Most functions in GCC should have a comment describing the arguments they
take as well as what they do, so I suppose I'd prefer to see something like:

/* Print operand X to file F in a target specific manner according to CODE.
   The acceptable formatting commands given by CODE are:
   [...]

> +     'c':		An integer or symbol address without a preceding # sign.
> +     'e':		Print the sign/zero-extend size as a character 8->b,
> +			16->h, 32->w.
> +     'p':		Prints N such that 2^N == X (X must be power of 2 and
> +			const int).
> +     'P':		Print the number of non-zero bits in X (a const_int).
> +     'H':		Print the higher numbered register of a pair (TImode)
> +			of regs.
> +     'm':		Print a condition (eq, ne, etc).
> +     'M':		Same as 'm', but invert condition.
> +     'b/q/h/s/d':	Print a scalar FP/SIMD register name. 

Put these in size order - b/h/s/d/q

> +     'S/T/U/V':		Print the first FP/SIMD register name in a list.

It might be useful to expand in this comment what the difference is between
S T U and V.

> +     'R':		Print a scalar FP/SIMD register name + 1.
> +     'X':		Print bottom 16 bits of integer constant in hex.
> +     'w/x':		Print a general register name or the zero register
> +			(32-bit or 64-bit).
> +     '0':		Print a normal operand, if it's a general register,
> +			then we assume DImode.
> +     'k':		Print nzcv.

This one doesn't make sense to me and could do with some clarification. Maybe
Print the <nzcv> field for CCMP.

Thanks,
James

> +     'A':		Output address constant representing the first
> +			argument of X, specifying a relocation offset
> +			if appropriate.
> +     'L':		Output constant address specified by X
> +			with a relocation offset if appropriate.
> +     'G':		Prints address of X, specifying a PC relative
> +			relocation mode if appropriate.  */
> +