* [PATCH] final: Improve output for -dp and -fverbose-asm
@ 2017-11-29 23:37 Segher Boessenkool
2017-11-30 7:52 ` Martin Sebor
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Segher Boessenkool @ 2017-11-29 23:37 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool
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(-)
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-29 23:37 [PATCH] final: Improve output for -dp and -fverbose-asm Segher Boessenkool
@ 2017-11-30 7:52 ` Martin Sebor
2017-11-30 11:54 ` Segher Boessenkool
2017-11-30 16:44 ` Kyrill Tkachov
2017-12-01 1:17 ` Jeff Law
2 siblings, 1 reply; 26+ messages in thread
From: Martin Sebor @ 2017-11-30 7:52 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches
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);
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 7:52 ` Martin Sebor
@ 2017-11-30 11:54 ` Segher Boessenkool
2017-11-30 16:06 ` Michael Matz
2017-11-30 16:36 ` Martin Sebor
0 siblings, 2 replies; 26+ messages in thread
From: Segher Boessenkool @ 2017-11-30 11:54 UTC (permalink / raw)
To: Martin Sebor; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 11:54 ` Segher Boessenkool
@ 2017-11-30 16:06 ` Michael Matz
2017-11-30 16:36 ` Martin Sebor
1 sibling, 0 replies; 26+ messages in thread
From: Michael Matz @ 2017-11-30 16:06 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: Martin Sebor, gcc-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 11:54 ` Segher Boessenkool
2017-11-30 16:06 ` Michael Matz
@ 2017-11-30 16:36 ` Martin Sebor
2017-11-30 16:50 ` Segher Boessenkool
2017-12-01 0:49 ` Jeff Law
1 sibling, 2 replies; 26+ messages in thread
From: Martin Sebor @ 2017-11-30 16:36 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:36 ` Martin Sebor
@ 2017-11-30 16:50 ` Segher Boessenkool
2017-11-30 17:02 ` Martin Sebor
2017-12-01 0:49 ` Jeff Law
1 sibling, 1 reply; 26+ messages in thread
From: Segher Boessenkool @ 2017-11-30 16:50 UTC (permalink / raw)
To: Martin Sebor; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:50 ` Segher Boessenkool
@ 2017-11-30 17:02 ` Martin Sebor
2017-11-30 17:14 ` Michael Matz
2017-11-30 17:15 ` Segher Boessenkool
0 siblings, 2 replies; 26+ messages in thread
From: Martin Sebor @ 2017-11-30 17:02 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:02 ` Martin Sebor
@ 2017-11-30 17:14 ` Michael Matz
2017-11-30 17:55 ` Martin Sebor
2017-11-30 17:55 ` David Malcolm
2017-11-30 17:15 ` Segher Boessenkool
1 sibling, 2 replies; 26+ messages in thread
From: Michael Matz @ 2017-11-30 17:14 UTC (permalink / raw)
To: Martin Sebor; +Cc: Segher Boessenkool, gcc-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:14 ` Michael Matz
@ 2017-11-30 17:55 ` Martin Sebor
2017-12-01 0:32 ` Jeff Law
` (2 more replies)
2017-11-30 17:55 ` David Malcolm
1 sibling, 3 replies; 26+ messages in thread
From: Martin Sebor @ 2017-11-30 17:55 UTC (permalink / raw)
To: Michael Matz; +Cc: Segher Boessenkool, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:55 ` Martin Sebor
@ 2017-12-01 0:32 ` Jeff Law
2017-12-01 22:52 ` Segher Boessenkool
2017-12-04 12:39 ` Michael Matz
2 siblings, 0 replies; 26+ messages in thread
From: Jeff Law @ 2017-12-01 0:32 UTC (permalink / raw)
To: Martin Sebor, Michael Matz; +Cc: Segher Boessenkool, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:55 ` Martin Sebor
2017-12-01 0:32 ` Jeff Law
@ 2017-12-01 22:52 ` Segher Boessenkool
2017-12-04 12:39 ` Michael Matz
2 siblings, 0 replies; 26+ messages in thread
From: Segher Boessenkool @ 2017-12-01 22:52 UTC (permalink / raw)
To: Martin Sebor; +Cc: Michael Matz, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:55 ` Martin Sebor
2017-12-01 0:32 ` Jeff Law
2017-12-01 22:52 ` Segher Boessenkool
@ 2017-12-04 12:39 ` Michael Matz
2 siblings, 0 replies; 26+ messages in thread
From: Michael Matz @ 2017-12-04 12:39 UTC (permalink / raw)
To: Martin Sebor; +Cc: Segher Boessenkool, gcc-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:14 ` Michael Matz
2017-11-30 17:55 ` Martin Sebor
@ 2017-11-30 17:55 ` David Malcolm
2017-12-04 15:49 ` Michael Matz
1 sibling, 1 reply; 26+ messages in thread
From: David Malcolm @ 2017-11-30 17:55 UTC (permalink / raw)
To: Michael Matz, Martin Sebor; +Cc: Segher Boessenkool, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:55 ` David Malcolm
@ 2017-12-04 15:49 ` Michael Matz
0 siblings, 0 replies; 26+ messages in thread
From: Michael Matz @ 2017-12-04 15:49 UTC (permalink / raw)
To: David Malcolm; +Cc: Martin Sebor, Segher Boessenkool, gcc-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:02 ` Martin Sebor
2017-11-30 17:14 ` Michael Matz
@ 2017-11-30 17:15 ` Segher Boessenkool
2017-11-30 22:59 ` Martin Sebor
1 sibling, 1 reply; 26+ messages in thread
From: Segher Boessenkool @ 2017-11-30 17:15 UTC (permalink / raw)
To: Martin Sebor; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:15 ` Segher Boessenkool
@ 2017-11-30 22:59 ` Martin Sebor
2017-12-01 0:26 ` Jeff Law
0 siblings, 1 reply; 26+ messages in thread
From: Martin Sebor @ 2017-11-30 22:59 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 22:59 ` Martin Sebor
@ 2017-12-01 0:26 ` Jeff Law
0 siblings, 0 replies; 26+ messages in thread
From: Jeff Law @ 2017-12-01 0:26 UTC (permalink / raw)
To: Martin Sebor, Segher Boessenkool; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:36 ` Martin Sebor
2017-11-30 16:50 ` Segher Boessenkool
@ 2017-12-01 0:49 ` Jeff Law
2017-12-01 23:45 ` Segher Boessenkool
1 sibling, 1 reply; 26+ messages in thread
From: Jeff Law @ 2017-12-01 0:49 UTC (permalink / raw)
To: Martin Sebor, Segher Boessenkool; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-12-01 0:49 ` Jeff Law
@ 2017-12-01 23:45 ` Segher Boessenkool
0 siblings, 0 replies; 26+ messages in thread
From: Segher Boessenkool @ 2017-12-01 23:45 UTC (permalink / raw)
To: Jeff Law; +Cc: Martin Sebor, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-29 23:37 [PATCH] final: Improve output for -dp and -fverbose-asm Segher Boessenkool
2017-11-30 7:52 ` Martin Sebor
@ 2017-11-30 16:44 ` Kyrill Tkachov
2017-11-30 16:54 ` Michael Matz
2017-12-01 1:17 ` Jeff Law
2 siblings, 1 reply; 26+ messages in thread
From: Kyrill Tkachov @ 2017-11-30 16:44 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:44 ` Kyrill Tkachov
@ 2017-11-30 16:54 ` Michael Matz
2017-11-30 16:55 ` Kyrill Tkachov
0 siblings, 1 reply; 26+ messages in thread
From: Michael Matz @ 2017-11-30 16:54 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: Segher Boessenkool, gcc-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:54 ` Michael Matz
@ 2017-11-30 16:55 ` Kyrill Tkachov
2017-11-30 17:07 ` Michael Matz
2017-12-01 0:25 ` Jeff Law
0 siblings, 2 replies; 26+ messages in thread
From: Kyrill Tkachov @ 2017-11-30 16:55 UTC (permalink / raw)
To: Michael Matz; +Cc: Segher Boessenkool, gcc-patches
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:55 ` Kyrill Tkachov
@ 2017-11-30 17:07 ` Michael Matz
2017-12-01 0:22 ` Jeff Law
2017-12-01 0:25 ` Jeff Law
1 sibling, 1 reply; 26+ messages in thread
From: Michael Matz @ 2017-11-30 17:07 UTC (permalink / raw)
To: Kyrill Tkachov; +Cc: Segher Boessenkool, gcc-patches
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 ;)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 17:07 ` Michael Matz
@ 2017-12-01 0:22 ` Jeff Law
0 siblings, 0 replies; 26+ messages in thread
From: Jeff Law @ 2017-12-01 0:22 UTC (permalink / raw)
To: Michael Matz, Kyrill Tkachov; +Cc: Segher Boessenkool, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-30 16:55 ` Kyrill Tkachov
2017-11-30 17:07 ` Michael Matz
@ 2017-12-01 0:25 ` Jeff Law
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Law @ 2017-12-01 0:25 UTC (permalink / raw)
To: Kyrill Tkachov, Michael Matz; +Cc: Segher Boessenkool, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] final: Improve output for -dp and -fverbose-asm
2017-11-29 23:37 [PATCH] final: Improve output for -dp and -fverbose-asm Segher Boessenkool
2017-11-30 7:52 ` Martin Sebor
2017-11-30 16:44 ` Kyrill Tkachov
@ 2017-12-01 1:17 ` Jeff Law
2 siblings, 0 replies; 26+ messages in thread
From: Jeff Law @ 2017-12-01 1:17 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches
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
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-12-04 15:49 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-29 23:37 [PATCH] final: Improve output for -dp and -fverbose-asm Segher Boessenkool
2017-11-30 7:52 ` Martin Sebor
2017-11-30 11:54 ` Segher Boessenkool
2017-11-30 16:06 ` Michael Matz
2017-11-30 16:36 ` Martin Sebor
2017-11-30 16:50 ` Segher Boessenkool
2017-11-30 17:02 ` Martin Sebor
2017-11-30 17:14 ` Michael Matz
2017-11-30 17:55 ` Martin Sebor
2017-12-01 0:32 ` Jeff Law
2017-12-01 22:52 ` Segher Boessenkool
2017-12-04 12:39 ` Michael Matz
2017-11-30 17:55 ` David Malcolm
2017-12-04 15:49 ` Michael Matz
2017-11-30 17:15 ` Segher Boessenkool
2017-11-30 22:59 ` Martin Sebor
2017-12-01 0:26 ` Jeff Law
2017-12-01 0:49 ` Jeff Law
2017-12-01 23:45 ` Segher Boessenkool
2017-11-30 16:44 ` Kyrill Tkachov
2017-11-30 16:54 ` Michael Matz
2017-11-30 16:55 ` Kyrill Tkachov
2017-11-30 17:07 ` Michael Matz
2017-12-01 0:22 ` Jeff Law
2017-12-01 0:25 ` Jeff Law
2017-12-01 1:17 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).