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