[PING] Re: RFR(L): 8213084: Rework and enhance Print[Opto]Assembly output
Schmidt, Lutz
lutz.schmidt at sap.com
Thu May 16 09:55:28 UTC 2019
Hi Vladimir,
sorry for the delayed reaction on your comments.
- now it reads "static unsigned int instr_len()". This change added cpu/s390/assembler_s390.inline.hpp to the list of modified files.
- testing from my side will be via the submit repo (BuildId: 2019-05-15-1543576.lutz.schmidt.source, no failures). In addition, I added the patch to our internal builds so that our inhouse testing will cover it (no issues detected last night).
- All the "hsdis-" prefixes in the PrintAssemblyOptions are gone, as are "print-pc" and "print-bytes". The latter two were legacy anyway. I kept them for compatibility. But now, without the prefix, there is no compatibility anymore.
- Options parsing improvement will be done in a separate effort. I have created JDK-8223765 for that.
- there is a new webrev, based on the current jdk/jdk repo: https://cr.openjdk.java.net/~lucy/webrevs/8213084.03/
~thartmann:
The disabled code in disassembler_s390.cpp is something I would like to have. So far, I could not find time to make it work reliably. I would like to keep it in as a reminder and a template to build on.
Thanks,
Lutz
On 10.05.19, 23:16, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
Hi Lutz,
My comments are inlined below.
On 5/10/19 8:44 AM, Schmidt, Lutz wrote:
> Thank you, Vladimir!
> Please find my comments inline and let me know what you think.
> A new webrev with all the updates is here: https://cr.openjdk.java.net/~lucy/webrevs/8213084.02/
Found one more I missed last time:
assembler_s390.hpp: still singed return (on other platforms it was converted to unsigned):
static int instr_len(unsigned char *instr);
> Please note: the webrev is not based on the most current jdk/jdk! I do not like the idea to "hg pull -u" to a repo state which is known to be broken. Once jdk/jdk is repaired, I will update the webrev in-place (provided there were no serious clashes) and sent a short note.
NP. Please, provide final webrev when you can so that I can run these changes through our testing to
make sure no issues are present (especially in builds).
> Regards,
> Lutz
>
> On 09.05.19, 21:30, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
>
> Hi Lutz,
>
> Thank you for doing this great work.
>
> I have just small comments:
>
> x86_64.ad - empty change.
> File contains whitespace changes for formatting. Not visible in webrev.
Okay.
>
> nmethod.cpp - LUCY?
>
> + st->print_cr("LUCY: NULL-oop");
> + tty->print("LUCY NULL-oop");
> Oops. Leftover debugging output. Removed. Reads "NULL-oop" now.
Okay.
>
> nmethod.cpp - use PTR64_FORMAT instead of '0x%016lx'.
> Changed.
>
> vmreg.cpp - Use INTPTR_FORMAT instead of %ld for value().
> Changed.
>
> disassembler.* - LUCY_OBSOLETE?
>
> +#if defined(LUCY_OBSOLETE) // Used in SAPJVM only
> This is fancy code to step backwards in CISC instructions. Used to print a +/- range around a given instruction address. Works reasonably well on s390, will probably not work at all for x86. I could not finally decide to kick it out. But now I did. It's gone.
Okay.
>
> compilerDefinitions.hpp - I don't see where tier_digit() is used.
> I'm surprised myself. Introduced it and then made it obsolete. It's gone.
>
> disassembler.cpp - PrintAssemblyOptions. Why you need to have 'hsdis-' in all options values? You
> need to check for invalid value and print help output in such case - it will be very useful if you
> forgot a value spelling. Also add line for 'help' value.
>
> The hsdis- prefix existed before I started my work. I just kept it to not hurt anybody's feelings__. Actually, the prefix has a minor practical use. It guards the many "if (strstr(..." instructions from being executed if there is no use. I'm personally not emotionally attached to the hsdis- prefix. I can remove it if you (and the other reviewers) like. Not changed as of now. Awaiting your input.
It is a pain to type long values and annoying to type the same prefix. I think hsdis- prefix is
useless because PrintAssemblyOptions is used only for disassembler and there are no values which
don't have hsdis- prefix. This is not performance critical code to have a guard (check prefix).
And an other commented new line:
+ // ost->print_cr("PrintAssemblyOptions='%s'", options());
>
> Printing help text: There is an option (hsdis-help) to request help text printout. >
> Options parsing doesn't exist here. It's just string comparisons. If one of the predefined strings is found - fine. If not - so what. If you would like to detect unrecognized input, process_options() needs significantly more intelligence. I can do that, but would like to do it in a separate effort. Your opinion?
Got it. I forgot that PrintAssemblyOptions flag accepts string with *list* of values - you can't use
if-else or switch without complicating the code.
I noticed that PrintAssemblyOptions is defined as ccstr. Why it is not ccstrlist which should be use
here? I don't think next comment is correct for ccstr type:
http://hg.openjdk.java.net/jdk/jdk/file/ef73702a906e/src/hotspot/share/compiler/disassembler.cpp#l190
It would be nice to fix it but you can do it later if you don't want to add more changes.
>
> Do you need next commented lines:
>
> disassembler.cpp -
> +// ptrdiff_t _offset;
> Deleted.
>
> +// Output suppressed because it messes up disassembly.
> +// output()->print_cr("[Disassembling for mach='%s']", (const char*)arg);
> Uncommented, would like to keep it. Made the if condition permanently false.
>
> disassembler_s390.cpp -
> +// st->fill_to(((st->position()+3*tsize-1)/tsize)*tsize);
> Deleted.
>
> compile.cpp -
> +// st->print("# "); _tf->dump_on(st); st->cr();
> Uncommented.
>
>
> abstractDisassembler.cpp -
> // st->print("0x%016lx", *((julong*)here));
> st->print("0x%016lx", *((uintptr_t*)here));
> // st->print("0x%08x%08x", *((juint*)here), *((juint*)(here+4)));
> Commented lines are gone.
>
> abstractDisassembler.cpp - may be explicit cast (byte*)?:
>
> st->print("%2.2x", *byte);
> st->print("%2.2x", *pos);
> st->print("0x%02x", *here);
> Didn't see the need because the pointers are char* (= address) anyway. And, according to cppreference.com, std::byte is a C++17 feature. We are not there yet.
okay
>
> PTR64_FORMAT ?:
> st->print("0x%016lx", *((uintptr_t*)here));
> I'm kind of hesitant on that. Nice output alignment clearly depends on this to output exactly 18 characters. Changed other occurrences, so I changed this one as well.
Thanks,
Vladimir
>
>
> Thanks,
> Vladimir
>
> On 5/8/19 8:31 AM, Schmidt, Lutz wrote:
> > Dear Community,
> >
> > may I please request comments and reviews for this change? Thank you!
> >
> > I have created a new webrev which is based on the current jdk/jdk repo. There was some merge effort. The code which constitutes this patch was not changed. Here's the webrev link:
> > https://cr.openjdk.java.net/~lucy/webrevs/8213084.01/
> >
> > Regards,
> > Lutz
> >
> > On 11.04.19, 23:24, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
> >
> > Dear All,
> >
> > this topic was discussed back in Nov/Dec 2018:
> > http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-November/031552.html
> > http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-December/031641.html
> >
> > Purpose of the discussion was to find out if my ideas are at all regarded useful and desirable.
> > The result was mixed, some pro, some con. I let the input from back then influence my work of the last months. In particular, output verbosity can be controlled in a wide range now. In addition to the general -XX:+Print* switches, the amount of output can be adjusted by newly introduced -XX:PrintAssemblyOptions. Here is the list (with default settings):
> >
> > PrintAssemblyOptions help:
> > hsdis-print-raw test plugin by requesting raw output (deprecated)
> > hsdis-print-raw-xml test plugin by requesting raw xml (deprecated)
> > hsdis-print-pc turn off PC printing (on by default) (deprecated)
> > hsdis-print-bytes turn on instruction byte output (deprecated)
> >
> > hsdis-show-pc toggle printing current pc, currently ON
> > hsdis-show-offset toggle printing current offset, currently OFF
> > hsdis-show-bytes toggle printing instruction bytes, currently OFF
> > hsdis-show-data-hex toggle formatting data as hex, currently ON
> > hsdis-show-data-int toggle formatting data as int, currently OFF
> > hsdis-show-data-float toggle formatting data as float, currently OFF
> > hsdis-show-structs toggle compiler data structures, currently OFF
> > hsdis-show-comment toggle instruction comments, currently OFF
> > hsdis-show-block-comment toggle block comments, currently OFF
> > hsdis-align-instr toggle instruction alignment, currently OFF
> >
> > Finally, I have pushed my changes to a state where I can dare to request your comments and reviews. I would like to suggest and request that we first focus on the effects (i.e. the generated output) of the changes. Once we got that adjusted and accepted, we can check the actual implementation and add improvements there. Sounds like a plan? Here is what you get:
> >
> > The machine code generated by the JVM can be printed in three different formats:
> > - Hexadecimal.
> > This is basically a hex dump of the memory range containing the code.
> > This format is always available (PRODUCT and not-PRODUCT builds), regardless
> > of the availability of a disassembler library. It applies to all sorts of
> > code, be it blobs, stubs, compiled nmethods, ...
> > This format seems useless at first glance, but it is not. In an upcoming,
> > separate enhancement, the JVM will be made capable of reading files
> > containing such code blocks and disassembling them post mortem. The most
> > prominent example is an hs_err* file.
> > - Disassembled.
> > This is an assembly listing of the instructions as found in the memory range
> > occupied by the blob, stub, compiled nmethod ... As a prerequisite, a suitable
> > disassembler library (hsdis-<platform>.so) must be available at runtime.
> > Most often, that will only be the case in test environments. If no disassembler
> > library is available, hexadecimal output is used as fallback.
> > - OptoAssembly.
> > This is a meta code listing created only by the C2 compiler. As it is somewhat
> > closer to the Java code, it may be helpful in linking assembly code to Java code.
> >
> > All three formats can be merged with additional information, most prominently compiler-internal "knowledge" about blocks, related bytecodes, statistics counters, and much more.
> >
> > Following the code itself, compiler-internal data structures, like oop maps, relocations, scopes, dependencies, exception handlers, are printed to aid in debugging.
> >
> > The full set of information is available in non-PRODUCT builds. PRODUCT builds do not support OptoAssembly output. Data structures are unavailable as well.
> >
> > So how does the output actually look like? Here are a few small snippets (linuxx86_64) to give you an idea. The complete output of an entire C2-compiled method, in multiple verbosity variants, is available here:
> > http://cr.openjdk.java.net/~lucy/webrevs/8213084/
> >
> > OptoAssembly output for reference (always on with PrintAssembly):
> > =================================================================
> >
> > 036 B2: # out( B7 B3 ) <- in( B1 ) Freq: 1
> > 036 movl RBP, [RSI + #12 (8-bit)] # compressed ptr ! Field: java/lang/String.value (constant)
> > 039 movl R11, [RBP + #12 (8-bit)] # range
> > 03d NullCheck RBP
> >
> > 03d B3: # out( B6 B4 ) <- in( B2 ) Freq: 0.999999
> > 03d cmpl RDX, R11 # unsigned
> > 040 jnb,us B6 P=0.000000 C=5375.000000
> >
> > PrintAssembly with no disassembler library available:
> > =====================================================
> >
> > [Code]
> > [Entry Point]
> > 0x00007fc74d1d7b20: 448b 5608 49c1 e203 493b c20f 856f 69e7 ff90 9090 9090 9090 9090 9090 9090 9090
> > [Verified Entry Point]
> > 0x00007fc74d1d7b40: 8984 2400 a0fe ff55 4883 ec20 440f be5e 1445 85db 7521 8b6e 0c44 8b5d 0c41 3bd3
> > 0x00007fc74d1d7b60: 732c 0fb6 4415 1048 83c4 205d 4d8b 9728 0100 0041 8502 c348 8bee 8914 2444 895c
> > 0x00007fc74d1d7b80: 2404 be4d ffff ffe8 1483 e7ff 0f0b bee5 ffff ff89 5424 04e8 0483 e7ff 0f0b bef6
> > 0x00007fc74d1d7ba0: ffff ff89 5424 04e8 f482 e7ff 0f0b f4f4 f4f4 f4f4 f4f4 f4f4 f4f4 f4f4 f4f4 f4f4
> > [Exception Handler]
> > 0x00007fc74d1d7bc0: e95b 0df5 ffe8 0000 0000 4883 2c24 05e9 0c7d e7ff
> > [End]
> >
> > PrintAssembly with minimal verbosity:
> > =====================================
> >
> > 0x00007f0434b89bd6: mov 0xc(%rsi),%ebp
> > 0x00007f0434b89bd9: mov 0xc(%rbp),%r11d
> > 0x00007f0434b89bdd: cmp %r11d,%edx
> > 0x00007f0434b89be0: jae 0x00007f0434b89c0e
> >
> > PrintAssembly (previous plus code offsets from code begin):
> > ===========================================================
> >
> > 0x00007f63c11d7956 (+0x36): mov 0xc(%rsi),%ebp
> > 0x00007f63c11d7959 (+0x39): mov 0xc(%rbp),%r11d
> > 0x00007f63c11d795d (+0x3d): cmp %r11d,%edx
> > 0x00007f63c11d7960 (+0x40): jae 0x00007f63c11d798e
> >
> > PrintAssembly (previous plus block comments):
> > ===========================================================
> >
> > ;; B2: # out( B7 B3 ) <- in( B1 ) Freq: 1
> > 0x00007f48211d76d6 (+0x36): mov 0xc(%rsi),%ebp
> > 0x00007f48211d76d9 (+0x39): mov 0xc(%rbp),%r11d
> > ;; B3: # out( B6 B4 ) <- in( B2 ) Freq: 0.999999
> > 0x00007f48211d76dd (+0x3d): cmp %r11d,%edx
> > 0x00007f48211d76e0 (+0x40): jae 0x00007f48211d770e
> >
> > PrintAssembly (previous plus instruction comments):
> > ===========================================================
> >
> > ;; B2: # out( B7 B3 ) <- in( B1 ) Freq: 1
> > 0x00007fc3e11d7a56 (+0x36): mov 0xc(%rsi),%ebp ;*getfield value {reexecute=0 rethrow=0 return_oop=0}
> > ; - java.lang.String::charAt at 8 (line 702)
> > 0x00007fc3e11d7a59 (+0x39): mov 0xc(%rbp),%r11d ; implicit exception: dispatches to 0x00007fc3e11d7a9e
> > ;; B3: # out( B6 B4 ) <- in( B2 ) Freq: 0.999999
> > 0x00007fc3e11d7a5d (+0x3d): cmp %r11d,%edx
> > 0x00007fc3e11d7a60 (+0x40): jae 0x00007fc3e11d7a8e
> >
> > For completeness, here are the links to
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8213084
> > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8213084.00/
> >
> > But please, as mentioned above, first focus on the output. The nitty details of the implementation I would like to discuss after the output format has received some support.
> >
> > Thank you so much for your time!
> > Lutz
> >
> >
> >
> >
>
>
More information about the hotspot-compiler-dev
mailing list