diff mbox

[Aarch64] Refactor comments in aarch64_print_operand

Message ID 29867bb1-a28c-d4bb-0893-9151c4c54e68@foss.arm.com
State New
Headers show

Commit Message

Jackson Woodruff July 13, 2017, 3:35 p.m. UTC 
Hi James,

I've addressed the issues discussed below.

OK for trunk?

Jackson

On 07/13/2017 10:03 AM, James Greenhalgh wrote:
> 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.  */
>> +

Comments

James Greenhalgh July 13, 2017, 4:14 p.m. UTC | #1 
On Thu, Jul 13, 2017 at 04:35:55PM +0100, Jackson Woodruff wrote:
> Hi James,
> 
> I've addressed the issues discussed below.
> 
> OK for trunk?

I one last comment, otherwise, this looks good:

> +/* 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/h/s/d/q':	Print a scalar FP/SIMD register name.
> +     'S/T/U/V':		Print the first FP/SIMD register name in a list
> +     			(No difference between any of these options).

There is a slight difference between these options - You'd use them in a
in a pattern with a large integer mode like LD3 on a CImode value to print
the register list you want to load. For example:

  LD3 {v0.4s - v2.4s} [x0]

The register number you'll get by inspecting REGNO (x) will give you the
start of the register list - but we need to get the right number for the
end of the register list too. To find that offset, we take
(CODE - 'S'). It should be clear why for S/T/U/V this gives 0/1/2/3.

So this comment should read:

  Print a FP/SIMD register name for a register list.  The register
  printed is the FP/SIMD register name of X + 0/1/2/3 for S/T/U/V.

Or similar.

Thanks,
James
diff mbox

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 037339d431d80c49699446e548d6b2707883b6a8..989429a203aaeb72980b89ecc43adb736019afe6 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5053,12 +5053,41 @@  static const int aarch64_nzcv_codes[] =
   0		/* NV, Any.  */
 };
 
+/* 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/h/s/d/q':	Print a scalar FP/SIMD register name.
+     'S/T/U/V':		Print the first FP/SIMD register name in a list
+     			(No difference between any of these options).
+     '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 for conditional compare instructions.
+     '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.  */
+
 static void
 aarch64_print_operand (FILE *f, rtx x, int code)
 {
   switch (code)
     {
-    /* An integer or symbol address without a preceding # sign.  */
     case 'c':
       switch (GET_CODE (x))
 	{
@@ -5085,7 +5114,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       break;
 
     case 'e':
-      /* Print the sign/zero-extend size as a character 8->b, 16->h, 32->w.  */
       {
 	int n;
 
@@ -5118,7 +5146,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       {
 	int n;
 
-	/* Print N such that 2^N == X.  */
 	if (!CONST_INT_P (x) || (n = exact_log2 (INTVAL (x))) < 0)
 	  {
 	    output_operand_lossage ("invalid operand for '%%%c'", code);
@@ -5130,7 +5157,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       break;
 
     case 'P':
-      /* Print the number of non-zero bits in X (a const_int).  */
       if (!CONST_INT_P (x))
 	{
 	  output_operand_lossage ("invalid operand for '%%%c'", code);
@@ -5141,7 +5167,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       break;
 
     case 'H':
-      /* Print the higher numbered register of a pair (TImode) of regs.  */
       if (!REG_P (x) || !GP_REGNUM_P (REGNO (x) + 1))
 	{
 	  output_operand_lossage ("invalid operand for '%%%c'", code);
@@ -5155,8 +5180,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
     case 'm':
       {
         int cond_code;
-	/* Print a condition (eq, ne, etc) or its inverse.  */
-
 	/* CONST_TRUE_RTX means al/nv (al is the default, don't print it).  */
 	if (x == const_true_rtx)
 	  {
@@ -5184,7 +5207,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
     case 's':
     case 'd':
     case 'q':
-      /* Print a scalar FP/SIMD register name.  */
       if (!REG_P (x) || !FP_REGNUM_P (REGNO (x)))
 	{
 	  output_operand_lossage ("incompatible floating point / vector register operand for '%%%c'", code);
@@ -5197,7 +5219,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
     case 'T':
     case 'U':
     case 'V':
-      /* Print the first FP/SIMD register name in a list.  */
       if (!REG_P (x) || !FP_REGNUM_P (REGNO (x)))
 	{
 	  output_operand_lossage ("incompatible floating point / vector register operand for '%%%c'", code);
@@ -5207,7 +5228,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       break;
 
     case 'R':
-      /* Print a scalar FP/SIMD register name + 1.  */
       if (!REG_P (x) || !FP_REGNUM_P (REGNO (x)))
 	{
 	  output_operand_lossage ("incompatible floating point / vector register operand for '%%%c'", code);
@@ -5217,7 +5237,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       break;
 
     case 'X':
-      /* Print bottom 16 bits of integer constant in hex.  */
       if (!CONST_INT_P (x))
 	{
 	  output_operand_lossage ("invalid operand for '%%%c'", code);
@@ -5228,8 +5247,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
 
     case 'w':
     case 'x':
-      /* Print a general register name or the zero register (32-bit or
-         64-bit).  */
       if (x == const0_rtx
 	  || (CONST_DOUBLE_P (x) && aarch64_float_const_zero_rtx_p (x)))
 	{
@@ -5252,8 +5269,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       /* Fall through */
 
     case 0:
-      /* Print a normal operand, if it's a general register, then we
-	 assume DImode.  */
       if (x == NULL)
 	{
 	  output_operand_lossage ("missing operand");
@@ -5406,7 +5421,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
       break;
 
     case 'G':
-
       switch (aarch64_classify_symbolic_expression (x))
 	{
 	case SYMBOL_TLSLE24:
@@ -5421,7 +5435,6 @@  aarch64_print_operand (FILE *f, rtx x, int code)
     case 'k':
       {
 	HOST_WIDE_INT cond_code;
-	/* Print nzcv.  */
 
 	if (!CONST_INT_P (x))
 	  {