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

[James Greenhalgh]

On Fri, Jul 14, 2017 at 10:11:51AM +0100, Jackson Woodruff wrote:
> Hi James,
> 
> I have addressed the issues below.
> 
> OK for trunk?

This looks good to me, thanks for the patch.

I've committed this to trunk on your behalf as revision 250202.

Thanks,
James

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

[Jackson Woodruff]

Hi James,

I have addressed the issues below.

OK for trunk?

Jackson
On 07/13/2017 05:14 PM, James Greenhalgh wrote:

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 --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 
037339d431d80c49699446e548d6b2707883b6a8..e77054c5e23aa876add1629d9eb13347441718cb
 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -5053,12 +5053,42 @@ 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 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.
+ '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 +5115,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 +5147,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 +5158,6 @@ aarch64_print_operand (FILE *f, 

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

[James Greenhalgh]

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

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

[Jackson Woodruff]

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  

  * 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  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.  */
+


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 

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

[James Greenhalgh]

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  
> 
>  * 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  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.  */
> +

[Patch][Aarch64] Refactor comments in aarch64_print_operand

[Jackson Woodruff]

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?

Changelog:

gcc/

2017-07-04  Jackson Woodruff  

 * 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:
+ '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.
+ 'S/T/U/V':Print the first FP/SIMD register name in a list.
+ '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.
+ '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 +5112,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 +5144,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 +5155,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 +5165,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 +5178,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 +5205,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 +5217,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 +5226,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