| Message ID | ac47cf30cab2820e22831de664d8d42964705d88.1511996668.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
| Series | final: Improve output for -dp and -fverbose-asm | expand |
On 11/29/2017 04:13 PM, Segher Boessenkool wrote: > This improves the assembler output (for -dp and -fverbose-asm) in > several ways. It always prints the insn_cost. It does not print > "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one > to the instruction alternative number (everything else starts counting > those at 0, too). And finally, it tries to keep things lined up in > columns a bit better. > > Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? [c=NN l=NN] will be meaningless to those without some deeper insight into/experience with what's actually being printed. It might as well say [NN NN]. But such extreme terseness would seem to run counter to the documented purpose of -fverbose-asm to "Put extra commentary information in the generated assembly code to make it more readable." Looking further in the manual I don't see the [length=NN] bit mentioned (nor does your patch add a mention of it) so while the meaning of [length=NN] isn't exactly obvious, using [l=NN] instead certainly won't make it easier to read. Does the manual need updating? Martin > > > Segher > > > 2017-11-29 Segher Boessenkool <segher@kernel.crashing.org> > > * final.c (output_asm_name): Print insn_cost. Shorten output. Print > which_alternative instead of which_alternative + 1. > (output_asm_insn): Print an extra tab if the template is short. > > --- > gcc/final.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/gcc/final.c b/gcc/final.c > index fdae241..afb6906 100644 > --- a/gcc/final.c > +++ b/gcc/final.c > @@ -3469,16 +3469,20 @@ output_asm_name (void) > { > if (debug_insn) > { > - int num = INSN_CODE (debug_insn); > - fprintf (asm_out_file, "\t%s %d\t%s", > - ASM_COMMENT_START, INSN_UID (debug_insn), > - insn_data[num].name); > - if (insn_data[num].n_alternatives > 1) > - fprintf (asm_out_file, "/%d", which_alternative + 1); > + fprintf (asm_out_file, "\t%s %d\t", > + ASM_COMMENT_START, INSN_UID (debug_insn)); > > + fprintf (asm_out_file, "[c=%d", > + insn_cost (debug_insn, optimize_insn_for_speed_p ())); > if (HAVE_ATTR_length) > - fprintf (asm_out_file, "\t[length = %d]", > + fprintf (asm_out_file, " l=%d", > get_attr_length (debug_insn)); > + fprintf (asm_out_file, "] "); > + > + int num = INSN_CODE (debug_insn); > + fprintf (asm_out_file, "%s", insn_data[num].name); > + if (insn_data[num].n_alternatives > 1) > + fprintf (asm_out_file, "/%d", which_alternative); > > /* Clear this so only the first assembler insn > of any rtl insn will get the special comment for -dp. */ > @@ -3824,6 +3828,10 @@ output_asm_insn (const char *templ, rtx *operands) > putc (c, asm_out_file); > } > > + /* Try to keep the asm a bit more readable. */ > + if ((flag_verbose_asm || flag_print_asm_name) && strlen (templ) < 9) > + putc ('\t', asm_out_file); > + > /* Write out the variable names for operands, if we know them. */ > if (flag_verbose_asm) > output_asm_operand_names (operands, oporder, ops); >
On Wed, Nov 29, 2017 at 08:46:41PM -0700, Martin Sebor wrote: > On 11/29/2017 04:13 PM, Segher Boessenkool wrote: > >This improves the assembler output (for -dp and -fverbose-asm) in > >several ways. It always prints the insn_cost. It does not print > >"[length = NN]" but "[c=NN l=NN]", to save space. It does not add one > >to the instruction alternative number (everything else starts counting > >those at 0, too). And finally, it tries to keep things lined up in > >columns a bit better. > > > >Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? > > [c=NN l=NN] will be meaningless to those without some deeper > insight into/experience with what's actually being printed. > It might as well say [NN NN]. But such extreme terseness would Length isn't printed on all targets, fwiw. > seem to run counter to the documented purpose of -fverbose-asm > to "Put extra commentary information in the generated assembly > code to make it more readable." The point is that [length = 12] takes up an awful lot of space. The output of -fverbose-asm alread suffers from information overload. > Looking further in the manual I don't see the [length=NN] bit > mentioned (nor does your patch add a mention of it) so while > the meaning of [length=NN] isn't exactly obvious, using [l=NN] > instead certainly won't make it easier to read. Does the manual > need updating? Should -fverbose-asm print length (and cost) at all? They should be printed for -dp (which is for debugging the *compiler*), but are they very useful for -fverbose-asm (which is primarily for debugging the *user program*)? Segher
Hi, On Thu, 30 Nov 2017, Segher Boessenkool wrote: > > [c=NN l=NN] will be meaningless to those without some deeper insight > > into/experience with what's actually being printed. It might as well > > say [NN NN]. But such extreme terseness would > > Length isn't printed on all targets, fwiw. Assembler is mnemonic, I see no reason to make the meta info not be mnemonic as well. In that light [NN NN] is clearly less mnemonic than [c=NN l=NN] (order is clear), and [length=NN] is overly verbose. People looking at -fverbose-asm output don't need hand-holding. Ciao, Michael.
On 11/30/2017 04:16 AM, Segher Boessenkool wrote: > On Wed, Nov 29, 2017 at 08:46:41PM -0700, Martin Sebor wrote: >> On 11/29/2017 04:13 PM, Segher Boessenkool wrote: >>> This improves the assembler output (for -dp and -fverbose-asm) in >>> several ways. It always prints the insn_cost. It does not print >>> "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one >>> to the instruction alternative number (everything else starts counting >>> those at 0, too). And finally, it tries to keep things lined up in >>> columns a bit better. >>> >>> Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? >> >> [c=NN l=NN] will be meaningless to those without some deeper >> insight into/experience with what's actually being printed. >> It might as well say [NN NN]. But such extreme terseness would > > Length isn't printed on all targets, fwiw. > >> seem to run counter to the documented purpose of -fverbose-asm >> to "Put extra commentary information in the generated assembly >> code to make it more readable." > > The point is that [length = 12] takes up an awful lot of space. The > output of -fverbose-asm alread suffers from information overload. Amount of space vs amount of detail are two different concerns. If you feel that the output is overly detailed then adding even more to it won't help. Other than that, I don't think trading readability for space savings makes sense in a format whose main purpose is to be readable. If it's line length that's a concern then using spaces instead of tabs would make them look shorter. You can also remove the brackets and use length=NN to shave off a couple of bytes. Or print length only when it's not the typical default. Or print the hex encoding instead. >> Looking further in the manual I don't see the [length=NN] bit >> mentioned (nor does your patch add a mention of it) so while >> the meaning of [length=NN] isn't exactly obvious, using [l=NN] >> instead certainly won't make it easier to read. Does the manual >> need updating? > > Should -fverbose-asm print length (and cost) at all? They should be > printed for -dp (which is for debugging the *compiler*), but are they > very useful for -fverbose-asm (which is primarily for debugging the > *user program*)? I don't have the answers to these questions. What I can say is that using one letter abbreviations for short words is not going to make it more readable, on the contrary. len= would be okay. Martin
Hi Segher, On 29/11/17 23:13, Segher Boessenkool wrote: > This improves the assembler output (for -dp and -fverbose-asm) in > several ways. It always prints the insn_cost. It does not print > "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one > to the instruction alternative number (everything else starts counting > those at 0, too). And finally, it tries to keep things lined up in > columns a bit better. > > Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? > FWIW printing the cost would be hepful to me at the -dp level. I agree with Martin that 'c' and 'l' are too short but using 'len' for length would be acceptable. Kyrill > > Segher > > > 2017-11-29 Segher Boessenkool <segher@kernel.crashing.org> > > * final.c (output_asm_name): Print insn_cost. Shorten output. > Print > which_alternative instead of which_alternative + 1. > (output_asm_insn): Print an extra tab if the template is short. > > --- > gcc/final.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/gcc/final.c b/gcc/final.c > index fdae241..afb6906 100644 > --- a/gcc/final.c > +++ b/gcc/final.c > @@ -3469,16 +3469,20 @@ output_asm_name (void) > { > if (debug_insn) > { > - int num = INSN_CODE (debug_insn); > - fprintf (asm_out_file, "\t%s %d\t%s", > - ASM_COMMENT_START, INSN_UID (debug_insn), > - insn_data[num].name); > - if (insn_data[num].n_alternatives > 1) > - fprintf (asm_out_file, "/%d", which_alternative + 1); > + fprintf (asm_out_file, "\t%s %d\t", > + ASM_COMMENT_START, INSN_UID (debug_insn)); > > + fprintf (asm_out_file, "[c=%d", > + insn_cost (debug_insn, optimize_insn_for_speed_p ())); > if (HAVE_ATTR_length) > - fprintf (asm_out_file, "\t[length = %d]", > + fprintf (asm_out_file, " l=%d", > get_attr_length (debug_insn)); > + fprintf (asm_out_file, "] "); > + > + int num = INSN_CODE (debug_insn); > + fprintf (asm_out_file, "%s", insn_data[num].name); > + if (insn_data[num].n_alternatives > 1) > + fprintf (asm_out_file, "/%d", which_alternative); > > /* Clear this so only the first assembler insn > of any rtl insn will get the special comment for -dp. */ > @@ -3824,6 +3828,10 @@ output_asm_insn (const char *templ, rtx *operands) > putc (c, asm_out_file); > } > > + /* Try to keep the asm a bit more readable. */ > + if ((flag_verbose_asm || flag_print_asm_name) && strlen (templ) < 9) > + putc ('\t', asm_out_file); > + > /* Write out the variable names for operands, if we know them. */ > if (flag_verbose_asm) > output_asm_operand_names (operands, oporder, ops); > -- > 1.8.3.1 >
On Thu, Nov 30, 2017 at 09:28:44AM -0700, Martin Sebor wrote: > >The point is that [length = 12] takes up an awful lot of space. The > >output of -fverbose-asm alread suffers from information overload. > > Amount of space vs amount of detail are two different concerns. Yes. > If you feel that the output is overly detailed then adding even > more to it won't help. Other than that, I don't think trading > readability for space savings makes sense in a format whose main > purpose is to be readable. If it's line length that's a concern > then using spaces instead of tabs would make them look shorter. Trunk: === .L.yk: cmpdi 0,4,0 # 8 *movdi_internal2/1 [length = 4] beq 0,.L2 # 9 *rs6000.md:12754 [length = 4] srdi 9,3,32 # 11 lshrdi3 [length = 4] xor 9,9,3 # 12 *boolsi3 [length = 4] rldicl 9,9,0,32 # 13 zero_extendsidi2/2 [length = 4] cmpd 7,9,3 # 14 *cmpdi_signed [length = 4] beq 7,.L7 # 15 *rs6000.md:12754 [length = 4] .L5: mr 3,4 # 32 *movdi_internal64/3 [length = 4] blr # 53 simple_return [length = 4] .L2: lwz 9,0(4) # 28 zero_extendsidi2/1 [length = 4] trap # 29 trap [length = 4] .L7: addic 4,9,-1 # 47 *adddi3_imm_carry_m1 [length = 4] subfe 4,4,9 # 48 *subfdi3_carry_in_internal [length = 4] b .L5 # 58 jump [length = 4] === Patched: === .L.yk: cmpdi 0,4,0 # 8 [c=4 l=4] *movdi_internal2/0 beq 0,.L2 # 9 [c=4 l=4] *rs6000.md:12774 srdi 9,3,32 # 11 [c=4 l=4] lshrdi3 xor 9,9,3 # 12 [c=4 l=4] *boolsi3 rldicl 9,9,0,32 # 13 [c=4 l=4] zero_extendsidi2/1 cmpd 7,9,3 # 14 [c=4 l=4] *cmpdi_signed beq 7,.L7 # 15 [c=4 l=4] *rs6000.md:12774 .L5: mr 3,4 # 32 [c=4 l=4] *movdi_internal64/2 blr # 76 [c=4 l=4] simple_return .L2: lfiwzx 0,0,4 # 28 [c=8 l=4] zero_extendsidi2/2 trap # 29 [c=4 l=4] trap .L7: addic 4,9,-1 # 70 [c=4 l=4] *adddi3_imm_carry_m1 subfe 4,4,9 # 71 [c=4 l=4] *subfdi3_carry_in_internal b .L5 # 81 [c=4 l=4] jump === It is neither line length nor amt of info that makes the second one way better readable. Segher
Hi, On Thu, 30 Nov 2017, Kyrill Tkachov wrote: > > This improves the assembler output (for -dp and -fverbose-asm) in > > several ways. It always prints the insn_cost. It does not print > > "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one > > to the instruction alternative number (everything else starts counting > > those at 0, too). And finally, it tries to keep things lined up in > > columns a bit better. > > > > Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? > > FWIW printing the cost would be hepful to me at the -dp level. I agree > with Martin that 'c' and 'l' are too short but using 'len' for length > would be acceptable. Seriously? You'd have a problem to decipher c/l but not rldicl ? Ciao, Michael.
On 30/11/17 16:47, Michael Matz wrote: > Hi, > > On Thu, 30 Nov 2017, Kyrill Tkachov wrote: > >>> This improves the assembler output (for -dp and -fverbose-asm) in >>> several ways. It always prints the insn_cost. It does not print >>> "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one >>> to the instruction alternative number (everything else starts counting >>> those at 0, too). And finally, it tries to keep things lined up in >>> columns a bit better. >>> >>> Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? >> FWIW printing the cost would be hepful to me at the -dp level. I agree >> with Martin that 'c' and 'l' are too short but using 'len' for length >> would be acceptable. > Seriously? You'd have a problem to decipher c/l but not rldicl ? I don't know what rldicl means without looking it up on the Internet ;) Given how frequently I have to look at these dumps, I could get used to any encoding though. I don't find them too verbose for my purposes, but if saving space is a goal here then I won't object to keeping them as single characters Thanks, Kyrill > > Ciao, > Michael.
On 11/30/2017 09:44 AM, Segher Boessenkool wrote: > On Thu, Nov 30, 2017 at 09:28:44AM -0700, Martin Sebor wrote: >>> The point is that [length = 12] takes up an awful lot of space. The >>> output of -fverbose-asm alread suffers from information overload. >> >> Amount of space vs amount of detail are two different concerns. > > Yes. > >> If you feel that the output is overly detailed then adding even >> more to it won't help. Other than that, I don't think trading >> readability for space savings makes sense in a format whose main >> purpose is to be readable. If it's line length that's a concern >> then using spaces instead of tabs would make them look shorter. > > Trunk: > > === > .L.yk: > cmpdi 0,4,0 # 8 *movdi_internal2/1 [length = 4] > beq 0,.L2 # 9 *rs6000.md:12754 [length = 4] > srdi 9,3,32 # 11 lshrdi3 [length = 4] > xor 9,9,3 # 12 *boolsi3 [length = 4] > rldicl 9,9,0,32 # 13 zero_extendsidi2/2 [length = 4] > cmpd 7,9,3 # 14 *cmpdi_signed [length = 4] > beq 7,.L7 # 15 *rs6000.md:12754 [length = 4] > .L5: > mr 3,4 # 32 *movdi_internal64/3 [length = 4] > blr # 53 simple_return [length = 4] > .L2: > lwz 9,0(4) # 28 zero_extendsidi2/1 [length = 4] > trap # 29 trap [length = 4] > .L7: > addic 4,9,-1 # 47 *adddi3_imm_carry_m1 [length = 4] > subfe 4,4,9 # 48 *subfdi3_carry_in_internal [length = 4] > b .L5 # 58 jump [length = 4] > === > > Patched: > === > .L.yk: > cmpdi 0,4,0 # 8 [c=4 l=4] *movdi_internal2/0 > beq 0,.L2 # 9 [c=4 l=4] *rs6000.md:12774 > srdi 9,3,32 # 11 [c=4 l=4] lshrdi3 > xor 9,9,3 # 12 [c=4 l=4] *boolsi3 > rldicl 9,9,0,32 # 13 [c=4 l=4] zero_extendsidi2/1 > cmpd 7,9,3 # 14 [c=4 l=4] *cmpdi_signed > beq 7,.L7 # 15 [c=4 l=4] *rs6000.md:12774 > .L5: > mr 3,4 # 32 [c=4 l=4] *movdi_internal64/2 > blr # 76 [c=4 l=4] simple_return > .L2: > lfiwzx 0,0,4 # 28 [c=8 l=4] zero_extendsidi2/2 > trap # 29 [c=4 l=4] trap > .L7: > addic 4,9,-1 # 70 [c=4 l=4] *adddi3_imm_carry_m1 > subfe 4,4,9 # 71 [c=4 l=4] *subfdi3_carry_in_internal > b .L5 # 81 [c=4 l=4] jump > === > > It is neither line length nor amt of info that makes the second one > way better readable. The justification certainly makes it easier to read. But the abbreviations make it harder to interpret. [c=4 l=4] makes no sense to anyone not already familiar with what it means. There's nothing wrong with using mnemonics as long as they're well established and commonly understood. Absent that, they should be explained in some accessible document. Not everyone who reads the verbose assembly is familiar with GCC internals. Users read it to help debug problems in their code. They shouldn't have to also study GCC source code to understand what the contents mean. Martin
Hi, On Thu, 30 Nov 2017, Kyrill Tkachov wrote: > I don't know what rldicl means without looking it up on the Internet ;) :) (zero_extendsidi2/1 it seems, from Seghers dump, not that I would have known without) > Given how frequently I have to look at these dumps, I could get used to > any encoding though. I don't find them too verbose for my purposes, but > if saving space is a goal here then I won't object to keeping them as > single characters I personally like the new format much more than what we have (also because of the more sensible alignment, not having the variable-length pattern name first). Ciao, Michael. P.S: feeling slightly sorry for adding bikeshed discussions to this trivial topic ;)
Hi, On Thu, 30 Nov 2017, Martin Sebor wrote: > > addic 4,9,-1 # 70 [c=4 l=4] *adddi3_imm_carry_m1 > > subfe 4,4,9 # 71 [c=4 l=4] *subfdi3_carry_in_internal > > b .L5 # 81 [c=4 l=4] jump > > Not everyone who reads the verbose assembly is familiar with > GCC internals. Users read it to help debug problems in their > code. They shouldn't have to also study GCC source code to > understand what the contents mean. Um, I think that's a bit overactive. How would you know what adddi3_imm_carry_m1 really means without knowing the particular GCC backend? Or what the above magic number after # means? Or, for that matter, what "length" means? Could be byte-length, sure. But OTOH, for a RISC target it's always four, so why print it? The GCC developers surely meant cycle-length with that, nothing else makes sense. My point is, to interpret the asm dumps there's no way around having some knowledge and getting used to it. In addition I doubt they're used heavily to debug programs. Rather they're used to study the interaction between compiler and program (and potentially to find miscompilations or strangenesses the compiler emits). As such even -fverbose-asm is a low-level compiler debugging tool, not something for an end-user that needs stability or documentation. Ciao, Michael.
On Thu, Nov 30, 2017 at 09:54:26AM -0700, Martin Sebor wrote: > >It is neither line length nor amt of info that makes the second one > >way better readable. > > The justification certainly makes it easier to read. But > the abbreviations make it harder to interpret. [c=4 l=4] > makes no sense to anyone not already familiar with what > it means. > > There's nothing wrong with using mnemonics as long as they're > well established and commonly understood. Absent that, they > should be explained in some accessible document. > > Not everyone who reads the verbose assembly is familiar with > GCC internals. Users read it to help debug problems in their > code. They shouldn't have to also study GCC source code to > understand what the contents mean. This is the -dp output, I hardly ever use -fverbose-asm, it has been unreadable for ten years or so. -fverbose-asm looks like this: === .L.yk: # 81288.c:4: unsigned int *un = (f3 != 0) ? &t4 : 0; cmpdi 0,4,0 # tmp130, f3 beq 0,.L2 # # 81288.c:6: *un ^= t4; srdi 9,3,32 #, tmp131, t4 xor 9,9,3 #, tmp132, tmp131, t4 # 81288.c:7: if (*un == t4) rldicl 9,9,0,32 # tmp133, tmp132 # 81288.c:7: if (*un == t4) cmpd 7,9,3 # t4, tmp134, tmp133 beq 7,.L7 # .L5: # 81288.c:11: } mr 3,4 #, <retval> blr .L2: # 81288.c:6: *un ^= t4; lwz 9,0(4) # MEM[(unsigned int *)0B], _13 trap .L7: # 81288.c:8: f3 = !!t4; addic 4,9,-1 # tmp139, tmp133 subfe 4,4,9 # <retval>, tmp139, tmp133 b .L5 # === If we're okay with outputting that kind of stuff to *users*, then how bad is [c=8 l=4] for GCC developers? Heh. Segher
On Thu, 2017-11-30 at 18:02 +0100, Michael Matz wrote: > Hi, > > On Thu, 30 Nov 2017, Martin Sebor wrote: > > > > addic 4,9,-1 # 70 [c=4 l=4] *adddi3_imm_carry_m1 > > > subfe 4,4,9 # 71 [c=4 > > > l=4] *subfdi3_carry_in_internal > > > b .L5 # 81 [c=4 l=4] jump > > > > Not everyone who reads the verbose assembly is familiar with > > GCC internals. Users read it to help debug problems in their > > code. They shouldn't have to also study GCC source code to > > understand what the contents mean. > > Um, I think that's a bit overactive. How would you know what > adddi3_imm_carry_m1 really means without knowing the particular GCC > backend? Or what the above magic number after # means? > > Or, for that matter, what "length" means? Could be byte-length, > sure. > But OTOH, for a RISC target it's always four, so why print it? The > GCC > developers surely meant cycle-length with that, nothing else makes > sense. > > My point is, to interpret the asm dumps there's no way around having > some > knowledge and getting used to it. In addition I doubt they're used > heavily to debug programs. Rather they're used to study the > interaction > between compiler and program (and potentially to find miscompilations > or > strangenesses the compiler emits). As such even -fverbose-asm is a > low-level compiler debugging tool, not something for an end-user > that > needs stability or documentation. -fverbose-asm is on the border of compiler-debugging vs end-user usage. FWIW an improvement to -fverbose-asm was explicitly mentioned in the gcc 7 release notes: https://gcc.gnu.org/gcc-7/changes.html and I've seen at least some end-users comment favorably on that change; this was: https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01051.html which was originally a "-fasm-show-source" option. Dave
On 11/30/2017 10:02 AM, Michael Matz wrote: > Hi, > > On Thu, 30 Nov 2017, Martin Sebor wrote: > >>> addic 4,9,-1 # 70 [c=4 l=4] *adddi3_imm_carry_m1 >>> subfe 4,4,9 # 71 [c=4 l=4] *subfdi3_carry_in_internal >>> b .L5 # 81 [c=4 l=4] jump >> >> Not everyone who reads the verbose assembly is familiar with >> GCC internals. Users read it to help debug problems in their >> code. They shouldn't have to also study GCC source code to >> understand what the contents mean. > > Um, I think that's a bit overactive. How would you know what > adddi3_imm_carry_m1 really means without knowing the particular GCC > backend? Or what the above magic number after # means? adddi3_imm_carry_m1 seems (mostly) self-explanatory since it's built up of common assembly mnemonics. I confess I don't know what the number after # means, either on powerpc64 or on any other target. I'd say that just shows that not even full time GCC developers are (or can be expected to be) familiar with all GCC internals, and we shouldn't need to study the back end code to interpret basic things like # 7 in the output. > Or, for that matter, what "length" means? Could be byte-length, sure. > But OTOH, for a RISC target it's always four, so why print it? The GCC > developers surely meant cycle-length with that, nothing else makes sense. Heh. I thought it meant the length of the instruction in bytes, and it made perfect sense to me. Sounds like I misinterpreted it. Which suggests that it should be mentioned in the manual (whatever label it ends up with). With it documented (and the position on the line made clear), the length= or l= part could even be skipped altogether to save a few more bytes if that's important (I don't think it is in this case). > My point is, to interpret the asm dumps there's no way around having some > knowledge and getting used to it. In addition I doubt they're used > heavily to debug programs. Rather they're used to study the interaction > between compiler and program (and potentially to find miscompilations or > strangenesses the compiler emits). As such even -fverbose-asm is a > low-level compiler debugging tool, not something for an end-user that > needs stability or documentation. Sure, basic knowledge of the target assembly is prerequisite. That includes some familiarity with common mnemonics. But except for details whose purpose is to expose GCC internals, knowledge of GCC implementation details (or having to read GCC source code that prints this stuff) shouldn't be. The basic point I'm making is that shortening length=NN to l=NN is not an improvement to the readability of the output and is contrary both to the documented purpose of the -fverbose-asm option and Segher's objective for the patch. The convention used in the output is to use mnemonics, similar to the assembly code itself. One letter mnemonics aren't nearly as effective as those consisting of multiple letters. Does l stand for load, length, latency, or something else? That's why they are almost mever used. In contrast, ld is a known mnemonic for load, len for length, and lat(?) could with some effort be correctly interpreted as latency. This seems fairly elementary to me and I would have expected it to be non-controversial so I'm not sure why it's being challenged. Don't we want the output to be generally useful? What do we gain by adopting these terse abbreviations? Martin
On 11/30/2017 10:07 AM, Segher Boessenkool wrote: > On Thu, Nov 30, 2017 at 09:54:26AM -0700, Martin Sebor wrote: >>> It is neither line length nor amt of info that makes the second one >>> way better readable. >> >> The justification certainly makes it easier to read. But >> the abbreviations make it harder to interpret. [c=4 l=4] >> makes no sense to anyone not already familiar with what >> it means. >> >> There's nothing wrong with using mnemonics as long as they're >> well established and commonly understood. Absent that, they >> should be explained in some accessible document. >> >> Not everyone who reads the verbose assembly is familiar with >> GCC internals. Users read it to help debug problems in their >> code. They shouldn't have to also study GCC source code to >> understand what the contents mean. > > This is the -dp output, I hardly ever use -fverbose-asm, it has been > unreadable for ten years or so. > > -fverbose-asm looks like this: > === > .L.yk: > # 81288.c:4: unsigned int *un = (f3 != 0) ? &t4 : 0; > cmpdi 0,4,0 # tmp130, f3 > beq 0,.L2 # > # 81288.c:6: *un ^= t4; > srdi 9,3,32 #, tmp131, t4 > xor 9,9,3 #, tmp132, tmp131, t4 > # 81288.c:7: if (*un == t4) > rldicl 9,9,0,32 # tmp133, tmp132 > # 81288.c:7: if (*un == t4) > cmpd 7,9,3 # t4, tmp134, tmp133 > beq 7,.L7 # > .L5: > # 81288.c:11: } > mr 3,4 #, <retval> > blr > .L2: > # 81288.c:6: *un ^= t4; > lwz 9,0(4) # MEM[(unsigned int *)0B], _13 > trap > .L7: > # 81288.c:8: f3 = !!t4; > addic 4,9,-1 # tmp139, tmp133 > subfe 4,4,9 # <retval>, tmp139, tmp133 > b .L5 # > === > > If we're okay with outputting that kind of stuff to *users*, then how > bad is [c=8 l=4] for GCC developers? Heh. I don't know if the above is okay or not. What I do know is that [l=4] is not an improvement over [length = 4]. But I think there are ways to improve the readability while at the same time making the output more compact. I mentioned documenting the labels (whatever they may be) in the manual as one possibility. Another idea is to print a brief legend at the bottom of the file explaining what l= stands for. Yet another is to print a header at the top of every function with a label for each column (like in the top command), and then document what each column means in the manual by referring to the column headers. I'm sure there are others. Martin
On 11/30/2017 09:55 AM, Michael Matz wrote: > Hi, > > On Thu, 30 Nov 2017, Kyrill Tkachov wrote: > >> I don't know what rldicl means without looking it up on the Internet ;) > > :) (zero_extendsidi2/1 it seems, from Seghers dump, not that I would have > known without) Exactly :-) THe names often make it a hell of a lot easier for someone not familiar with a particular architecture to make out what a particular insn does. > >> Given how frequently I have to look at these dumps, I could get used to >> any encoding though. I don't find them too verbose for my purposes, but >> if saving space is a goal here then I won't object to keeping them as >> single characters > > I personally like the new format much more than what we have (also because > of the more sensible alignment, not having the variable-length pattern > name first). Likewise. I think we should go with the patch as-is. Jeff
On 11/30/2017 09:50 AM, Kyrill Tkachov wrote: > > On 30/11/17 16:47, Michael Matz wrote: >> Hi, >> >> On Thu, 30 Nov 2017, Kyrill Tkachov wrote: >> >>>> This improves the assembler output (for -dp and -fverbose-asm) >>>> in several ways. It always prints the insn_cost. It does not >>>> print "[length = NN]" but "[c=NN l=NN]", to save space. It >>>> does not add one to the instruction alternative number >>>> (everything else starts counting those at 0, too). And >>>> finally, it tries to keep things lined up in columns a bit >>>> better. >>>> >>>> Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? >>> FWIW printing the cost would be hepful to me at the -dp level. I >>> agree with Martin that 'c' and 'l' are too short but using 'len' >>> for length would be acceptable. >> Seriously? You'd have a problem to decipher c/l but not rldicl ? > > I don't know what rldicl means without looking it up on the Internet > ;) Given how frequently I have to look at these dumps, I could get > used to any encoding though. I don't find them too verbose for my > purposes, but if saving space is a goal here then I won't object to > keeping them as single characters ISTM that saving space is a goal if it generally makes the output more readable. As we include more data in the output we do need to consider the clutter factor. c= and l= seem reasonable to me. There's a level of familiarity with GCC that is necessary to fully interpret that output. However, users can still get an awful lot from that output without immediately knowing what each and every field looks like. jeff
On 11/30/2017 03:54 PM, Martin Sebor wrote: > On 11/30/2017 10:07 AM, Segher Boessenkool wrote: >> On Thu, Nov 30, 2017 at 09:54:26AM -0700, Martin Sebor wrote: >>>> It is neither line length nor amt of info that makes the second one >>>> way better readable. >>> >>> The justification certainly makes it easier to read. But >>> the abbreviations make it harder to interpret. [c=4 l=4] >>> makes no sense to anyone not already familiar with what >>> it means. >>> >>> There's nothing wrong with using mnemonics as long as they're >>> well established and commonly understood. Absent that, they >>> should be explained in some accessible document. >>> >>> Not everyone who reads the verbose assembly is familiar with >>> GCC internals. Users read it to help debug problems in their >>> code. They shouldn't have to also study GCC source code to >>> understand what the contents mean. >> >> This is the -dp output, I hardly ever use -fverbose-asm, it has been >> unreadable for ten years or so. >> >> -fverbose-asm looks like this: >> === >> .L.yk: >> # 81288.c:4: unsigned int *un = (f3 != 0) ? &t4 : 0; >> cmpdi 0,4,0 # tmp130, f3 >> beq 0,.L2 # >> # 81288.c:6: *un ^= t4; >> srdi 9,3,32 #, tmp131, t4 >> xor 9,9,3 #, tmp132, tmp131, t4 >> # 81288.c:7: if (*un == t4) >> rldicl 9,9,0,32 # tmp133, tmp132 >> # 81288.c:7: if (*un == t4) >> cmpd 7,9,3 # t4, tmp134, tmp133 >> beq 7,.L7 # >> .L5: >> # 81288.c:11: } >> mr 3,4 #, <retval> >> blr >> .L2: >> # 81288.c:6: *un ^= t4; >> lwz 9,0(4) # MEM[(unsigned int *)0B], _13 >> trap >> .L7: >> # 81288.c:8: f3 = !!t4; >> addic 4,9,-1 # tmp139, tmp133 >> subfe 4,4,9 # <retval>, tmp139, tmp133 >> b .L5 # >> === >> >> If we're okay with outputting that kind of stuff to *users*, then how >> bad is [c=8 l=4] for GCC developers? Heh. > > I don't know if the above is okay or not. What I do know is > that [l=4] is not an improvement over [length = 4]. It can be if the lines are getting long enough to wrap. > > But I think there are ways to improve the readability while > at the same time making the output more compact. I mentioned > documenting the labels (whatever they may be) in the manual > as one possibility. Another idea is to print a brief legend > at the bottom of the file explaining what l= stands for. Yet > another is to print a header at the top of every function with > a label for each column (like in the top command), and then > document what each column means in the manual by referring > to the column headers. I'm sure there are others. And I think these could all move forward independently. jeff
On 11/30/2017 10:55 AM, Martin Sebor wrote: > On 11/30/2017 10:02 AM, Michael Matz wrote: >> Hi, >> >> On Thu, 30 Nov 2017, Martin Sebor wrote: >> >>>> addic 4,9,-1 # 70 [c=4 l=4] *adddi3_imm_carry_m1 >>>> subfe 4,4,9 # 71 [c=4 l=4] *subfdi3_carry_in_internal >>>> b .L5 # 81 [c=4 l=4] jump >>> >>> Not everyone who reads the verbose assembly is familiar with >>> GCC internals. Users read it to help debug problems in their >>> code. They shouldn't have to also study GCC source code to >>> understand what the contents mean. >> >> Um, I think that's a bit overactive. How would you know what >> adddi3_imm_carry_m1 really means without knowing the particular GCC >> backend? Or what the above magic number after # means? > > adddi3_imm_carry_m1 seems (mostly) self-explanatory since it's > built up of common assembly mnemonics. I confess I don't know > what the number after # means, either on powerpc64 or on any > other target. I'd say that just shows that not even full time > GCC developers are (or can be expected to be) familiar with all > GCC internals, and we shouldn't need to study the back end code > to interpret basic things like # 7 in the output. FTR the number the instruction is the insn's UID :-) Also note that whether or not an instruction has a human readable mnemonic is dependent on the target -- many don't have names for the bulk of their insns or the names are fairly cryptic. Ultimately fully understanding that data will always require a fairly good understanding of GCC and the target. > >> Or, for that matter, what "length" means? Could be byte-length, sure. >> But OTOH, for a RISC target it's always four, so why print it? The GCC >> developers surely meant cycle-length with that, nothing else makes sense. > > Heh. I thought it meant the length of the instruction in bytes, > and it made perfect sense to me. Sounds like I misinterpreted it. > Which suggests that it should be mentioned in the manual (whatever > label it ends up with). With it documented (and the position on > the line made clear), the length= or l= part could even be skipped > altogether to save a few more bytes if that's important (I don't > think it is in this case). It's *supposed* to be bytes. However, some targets may not have made the conversion from instructions to bytes. Jeff
On 11/30/2017 09:28 AM, Martin Sebor wrote: > On 11/30/2017 04:16 AM, Segher Boessenkool wrote: >> On Wed, Nov 29, 2017 at 08:46:41PM -0700, Martin Sebor wrote: >>> On 11/29/2017 04:13 PM, Segher Boessenkool wrote: >>>> This improves the assembler output (for -dp and -fverbose-asm) in >>>> several ways. It always prints the insn_cost. It does not print >>>> "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one >>>> to the instruction alternative number (everything else starts counting >>>> those at 0, too). And finally, it tries to keep things lined up in >>>> columns a bit better. >>>> >>>> Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? >>> >>> [c=NN l=NN] will be meaningless to those without some deeper >>> insight into/experience with what's actually being printed. >>> It might as well say [NN NN]. But such extreme terseness would >> >> Length isn't printed on all targets, fwiw. >> >>> seem to run counter to the documented purpose of -fverbose-asm >>> to "Put extra commentary information in the generated assembly >>> code to make it more readable." >> >> The point is that [length = 12] takes up an awful lot of space. The >> output of -fverbose-asm alread suffers from information overload. > > Amount of space vs amount of detail are two different concerns. > If you feel that the output is overly detailed then adding even > more to it won't help. Other than that, I don't think trading > readability for space savings makes sense in a format whose main > purpose is to be readable. If it's line length that's a concern > then using spaces instead of tabs would make them look shorter. > You can also remove the brackets and use length=NN to shave off > a couple of bytes. Or print length only when it's not the typical > default. Or print the hex encoding instead. Some of those things may make sense as well. Though we also have to be careful because some points don't have length information at all. SIgh. > >>> Looking further in the manual I don't see the [length=NN] bit >>> mentioned (nor does your patch add a mention of it) so while >>> the meaning of [length=NN] isn't exactly obvious, using [l=NN] >>> instead certainly won't make it easier to read. Does the manual >>> need updating? >> >> Should -fverbose-asm print length (and cost) at all? They should be >> printed for -dp (which is for debugging the *compiler*), but are they >> very useful for -fverbose-asm (which is primarily for debugging the >> *user program*)? > > I don't have the answers to these questions. What I can say > is that using one letter abbreviations for short words is not > going to make it more readable, on the contrary. len= would > be okay. I think length and costing information are definitely things we want to include. Length is less of an issue now than it was in the past, but it definitely has value. jeff
On 11/29/2017 04:13 PM, Segher Boessenkool wrote: > This improves the assembler output (for -dp and -fverbose-asm) in > several ways. It always prints the insn_cost. It does not print > "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one > to the instruction alternative number (everything else starts counting > those at 0, too). And finally, it tries to keep things lined up in > columns a bit better. > > Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? > > > Segher > > > 2017-11-29 Segher Boessenkool <segher@kernel.crashing.org> > > * final.c (output_asm_name): Print insn_cost. Shorten output. Print > which_alternative instead of which_alternative + 1. > (output_asm_insn): Print an extra tab if the template is short. OK. jeff
Hi! On Thu, Nov 30, 2017 at 10:55:04AM -0700, Martin Sebor wrote: > >Or, for that matter, what "length" means? Could be byte-length, sure. > >But OTOH, for a RISC target it's always four, so why print it? The GCC > >developers surely meant cycle-length with that, nothing else makes sense. > > Heh. I thought it meant the length of the instruction in bytes, > and it made perfect sense to me. Sounds like I misinterpreted it. It is: "Lengths are measured in addressable storage units (bytes)." (which is in the manual just fine; gccint of course, not the user manual). > Which suggests that it should be mentioned in the manual (whatever > label it ends up with). With it documented (and the position on > the line made clear), the length= or l= part could even be skipped > altogether to save a few more bytes if that's important (I don't > think it is in this case). It is documented with -dp (I'll document it prints insn cost too). Segher
On Thu, Nov 30, 2017 at 05:49:27PM -0700, Jeff Law wrote: > I think length and costing information are definitely things we want to > include. Length is less of an issue now than it was in the past, but it > definitely has value. At least for risc targets length is usually pretty boring, but this is not the length of machine insns, RTL insns instead, making it more insteresting; and when it is wrong it leads to hard to debug problems, and we don't have this info easily accessible elsewhere. Similar goes for the insn_cost: if you need to debug it, the -dp output is a very convenient place to quickly get an overview of what we generate. Segher
Hi, On Thu, 30 Nov 2017, Martin Sebor wrote: > adddi3_imm_carry_m1 seems (mostly) self-explanatory since it's built up > of common assembly mnemonics. I confess I don't know what the number > after # means, either on powerpc64 or on any other target. insn uid. > > Or, for that matter, what "length" means? Could be byte-length, sure. > > But OTOH, for a RISC target it's always four, so why print it? The > > GCC developers surely meant cycle-length with that, nothing else makes > > sense. > > Heh. I thought it meant the length of the instruction in bytes, and it > made perfect sense to me. It _does_. My point was to show you that even lengthy (ahem) non-abbreviations are open to interpretation, and that it's not the number of characters that make the difference, but knowledge. And perhaps, missing the latter, documentation. > The basic point I'm making is that shortening length=NN to l=NN > is not an improvement to the readability of the output And I disagree. It _is_ improving readability IMHO. Basically the more often you need to mention token X, the shorter it can and should be. If you mention something every line, it should be as short as possible, which is one character, and to give the eye some hold while scanning the line some visual punctuation like '=' should be added as well. > as those consisting of multiple letters. Does l stand for load, > length, latency, or something else? As context matters I think you're making up a problem that doesn't exist. > This seems fairly elementary to me and I would have expected > it to be non-controversial so I'm not sure why it's being > challenged. Don't we want the output to be generally useful? Define "generally useful" in the context of -fverbose-asm. I think it is already, and Seghers patch improves on this. > What do we gain by adopting these terse abbreviations? That OTOH seems obvious to me: lines that don't exceed terminal width. There's nothing more disturbing than line breaks in line-oriented formats like assembler. Ciao, Michael.
Hi, On Thu, 30 Nov 2017, David Malcolm wrote: > -fverbose-asm is on the border of compiler-debugging vs end-user usage. I have yet to see a relevant percentage of end-users who even know what assembler is. On the gcc.*@ and kernel.* mailing lists, sure. But Joe Randomapp? > FWIW an improvement to -fverbose-asm was explicitly mentioned in the > gcc 7 release notes: > https://gcc.gnu.org/gcc-7/changes.html > and I've seen at least some end-users comment favorably on that change; > this was: > https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01051.html > which was originally a "-fasm-show-source" option. Yes. I think this was actually a disservice to readability of -fverbose-asm (sorry!) and would have preferred a suboption as well (but wouldn't have complained if with-sources would be the default). First, it clutters the asm instructions with intervening non-aligned lines (and left-hanging even, giving more visual importance to them instead of what is actually important, which for a switch named verbose-asm seems the asm to me) and second it has the same problem as debugging scheduled code: jumping around crazy and stating the same source line multiple times. (Basically the same reason 'objdump -dS' is similarly ugly, and why the -S therein is an extra switch). Luckily -dp still works as expected, so, ... well, I guess I'll live, and if not there's "grep -v '^#.*:[0-9]\+:'" :) Ciao, Michael.
diff --git a/gcc/final.c b/gcc/final.c index fdae241..afb6906 100644 --- a/gcc/final.c +++ b/gcc/final.c @@ -3469,16 +3469,20 @@ output_asm_name (void) { if (debug_insn) { - int num = INSN_CODE (debug_insn); - fprintf (asm_out_file, "\t%s %d\t%s", - ASM_COMMENT_START, INSN_UID (debug_insn), - insn_data[num].name); - if (insn_data[num].n_alternatives > 1) - fprintf (asm_out_file, "/%d", which_alternative + 1); + fprintf (asm_out_file, "\t%s %d\t", + ASM_COMMENT_START, INSN_UID (debug_insn)); + fprintf (asm_out_file, "[c=%d", + insn_cost (debug_insn, optimize_insn_for_speed_p ())); if (HAVE_ATTR_length) - fprintf (asm_out_file, "\t[length = %d]", + fprintf (asm_out_file, " l=%d", get_attr_length (debug_insn)); + fprintf (asm_out_file, "] "); + + int num = INSN_CODE (debug_insn); + fprintf (asm_out_file, "%s", insn_data[num].name); + if (insn_data[num].n_alternatives > 1) + fprintf (asm_out_file, "/%d", which_alternative); /* Clear this so only the first assembler insn of any rtl insn will get the special comment for -dp. */ @@ -3824,6 +3828,10 @@ output_asm_insn (const char *templ, rtx *operands) putc (c, asm_out_file); } + /* Try to keep the asm a bit more readable. */ + if ((flag_verbose_asm || flag_print_asm_name) && strlen (templ) < 9) + putc ('\t', asm_out_file); + /* Write out the variable names for operands, if we know them. */ if (flag_verbose_asm) output_asm_operand_names (operands, oporder, ops);
This improves the assembler output (for -dp and -fverbose-asm) in several ways. It always prints the insn_cost. It does not print "[length = NN]" but "[c=NN l=NN]", to save space. It does not add one to the instruction alternative number (everything else starts counting those at 0, too). And finally, it tries to keep things lined up in columns a bit better. Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? Segher 2017-11-29 Segher Boessenkool <segher@kernel.crashing.org> * final.c (output_asm_name): Print insn_cost. Shorten output. Print which_alternative instead of which_alternative + 1. (output_asm_insn): Print an extra tab if the template is short. --- gcc/final.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)