RFR(S): 8139046: Compiler Control: IVGPrintLevel directive should set PrintIdealGraph
Nils Eliasson
nils.eliasson at oracle.com
Thu Jun 11 13:46:30 UTC 2020
Hi Xin,
src/hotspot/share/opto/c2_globals.hpp
367 notproduct(intx, PrintIdealGraphLevel, 0, \
368 "Level of detail of the ideal graph printout. " \
369 "System-wide value, -1=absolutely nothing is printed, " \
370 "0=nothing except IGVPrintLevel directives, 4=all details printed. " \
371 "Level of detail of printouts can be set on a per-method level " \
372 "as well by using CompileCommand=option.") \
373 range(-1, 4)
Can you change the text for -1 to "printing is disabled" - because that
is the practical difference between 0 and -1.
Otherwise - it looks good.
For a future fix I would suggest creating the printer on demand. Then
the -1 value wouldn't be needed anymore.
Regards,
Nils Eliasson
On 2020-06-09 00:59, Liu, Xin wrote:
> Hi, Paul and Volker,
>
> Yes, it's misleading to set PrintIdealGraph true by default. I set it back and here is a new revision:
> http://cr.openjdk.java.net/~xliu/8139046/02/webrev/
>
> PrintIdealGraphLevel=0 doesn't work because users might use compiler directive IGVPrintLevel > 0.
> I extent the range of PrintIdealGraphLevel a little bit. if PrintIdealGraphLevel=-1, it means user don't want to have any Ideal Graph dumped. I make -XX:-PrintIdealGraph a synonym of PrintIdealGraphLevel == -1.
>
> The feature is useful when developers start using a lot of compiler directives with IGVPrintLevel. Without it, developers have to modify many directives to turn off PrintIdealGraph.
>
> I still think PrintIdealGraph should go away because PrintIdealGraphLevel can do better job. As Paul suggested, PrintIdealGraph has been wiped out from share/opto directory.
> Here is the new comment of PrintIdealGraphLevel.
> intx PrintIdealGraphLevel = 0 {C2 notproduct} {default} Level of detail of the ideal graph printout. System-wide value, -1=absolutely nothing is printed, 0=nothing except IGVPrintLevel directives, 4=all details printed. Level of detail of printouts can be set on a per-method level as well by using CompileCommand=option.
>
> Thanks,
> --lx
>
>
> On 6/5/20, 6:38 AM, "Hohensee, Paul" <hohensee at amazon.com> wrote:
>
> Re PrintIdealGraph, I'd consider removing its remaining reference in compile.hpp in favor of
>
> 1. Leave the default value of PrintIdealGraph at false. It's confusing to see it set true by default.
>
> 2. Make -XX:-PrintIdealGraph a synonym for PrintIdealGraphLevel == 0. I.e., set PrintIdealGraphLevel to 0 in ergo_initialize() if PrintIdealGraph was explicitly set false on the command line. Use
>
> if (FLAG_IS_CMDLINE(PrintIdealGraph) && !PrintIdealGraph)) {
> FLAG_SET_ERGO(PrintIdealGraphLevel, 0);
> }
>
> 3. Ignore PrintIdealGraph otherwise, which is what happens now (i.e., +PrintIdealGraph has no effect if PrintIdealGraphLevel == 0, which it is by default). Have should_print() in compile.hpp test for PrintIdealGraphLevel == 0 instead of !PrintIdealGraph.
>
> That way, a future deprecation/removal of PrintIdealGraph is isolated. And, you file an RFE to do that.
>
> Thanks,
> Paul
>
> On 6/4/20, 6:06 PM, "hotspot-compiler-dev on behalf of Liu, Xin" <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of xxinliu at amazon.com> wrote:
>
> Hi, Volker,
>
> Thank you to review it.
>
> - Why do you need the extra check for "method() != NULL":
> Yes, I try to avoid a corner case. Previously, _printer is set to NULL when c2 compiles runtime stubs(http://hg.openjdk.java.net/jdk/jdk/file/b06f452c8d61/src/hotspot/share/opto/compile.cpp#l825).
> That will cause a problem for my patch because I initialize _printer to NULL and create it when we do need to print IdealGraph.
> If users specify PrintIdealGraphLevel>0, C2 will try to dump a NULL method. It is undefined to invoke begin/end_method() if the C->method() is NULL, so it better guard with the NULL check.
>
> Here is the context:
> "
> Command Line: -XX:PrintIdealGraphLevel=3 -XX:PrintIdealGraphFile=hello.xml
>
> Host: ip-172-31-94-125, Intel(R) Xeon(R) Platinum 8175M CPU @ 2.50GHz, 48 cores, 184G, Ubuntu 18.04.4 LTS
> Time: Thu Jun 4 23:25:00 2020 UTC elapsed time: 0.150427 seconds (0d 0h 0m 0s)
>
> --------------- T H R E A D ---------------
>
> Current thread (0x00007f69a83f3150): JavaThread "C2 CompilerThread0" daemon [_thread_in_vm, id=32990, stack(0x00007f692bc8b000,0x00007f692bd8c000)]
>
> Stack: [0x00007f692bc8b000,0x00007f692bd8c000], sp=0x00007f692bd893c0, free space=1016k
> Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
> V [libjvm.so+0xc17c94] IdealGraphPrinter::begin_method()+0x434
> V [libjvm.so+0x88e5b1] CompileWrapper::CompileWrapper(Compile*)+0x201
> V [libjvm.so+0x89f689] Compile::Compile(ciEnv*, TypeFunc const* (*)(), unsigned char*, char const*, int, bool, bool, bool, DirectiveSet*)+0x4c9
> V [libjvm.so+0x1425586] OptoRuntime::generate_stub(ciEnv*, TypeFunc const* (*)(), unsigned char*, char const*, int, bool, bool, bool)+0x156
> "
>
> I took all your advices except for that.
> Could you take a look at the new revision:
> http://cr.openjdk.java.net/~xliu/8139046/01/webrev/
>
> To get rid of the param 'bool need', I set PrintIdealGraph true by default. Previously, if PrintIdealGraph is set, hotspot will initialize a printer object for every Compiler thread. It's not true anymore, so no extra cost.
> After this patch, the only usage of that flag is to shut down IGVPrinter completely by -XX:-PrintIdealGraph. I wish we can vote it out in the future and use PrintIdealGraphLevel.
>
> Thanks,
> --lx
>
>
>
> From: Volker Simonis <volker.simonis at gmail.com>
> Date: Thursday, June 4, 2020 at 10:13 AM
> To: "Liu, Xin" <xxinliu at amazon.com>
> Cc: "hotspot-compiler-dev at openjdk.java.net" <hotspot-compiler-dev at openjdk.java.net>
> Subject: RE: [EXTERNAL] RFR(S): 8139046: Compiler Control: IVGPrintLevel directive should set PrintIdealGraph
>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
> Hi Xin,
>
> Thanks for addressing this issue. It looks like a nice cleanup. Please find my further comments inline:
>
> On Tue, Jun 2, 2020 at 10:57 PM Liu, Xin <mailto:xxinliu at amazon.com> wrote:
> Hi,
>
> Could you review this webrev? It fixes a minor problem when users only use IGVPrintLevel in Compiler Directives.
> Jbs: https://bugs.openjdk.java.net/browse/JDK-8139046
> Webrev: http://cr.openjdk.java.net/~xliu/8139046/00/webrev/
>
> I move "bool should_print(int level)" from idealGraphPrinter to Compile because the later has the information.
> In this way, Compile can allocate _printer on demand. If Compile::should_print(level) return true, it guarantees that Compile::printer() is not NULL.
> If users pass in CompileCommand="option,Hello::add,intx,IGVPrintLevel,3", printer() will only turn on for that compiler thread.
>
> - Why do you need the extra check for "method() != NULL":
> 619 if (should_print(1) && method() != NULL) {
> 4584 if (should_print(level) && method() != NULL) {
> in "Compile::{begin,end_method}". This check wasn't there before. Does it fix an issue?
>
> - I don't see why you need the additional "need" parameter in "IdealGraphPrinter::printer()". The function only gets called with "need == true" anyway, so I think you can remove it.
>
> - Why did you make
> 453 IdealGraphPrinter* printer() { return _printer; }
> a "const" function?
> 453 IdealGraphPrinter* printer() const { return _printer; }
> I don't think it is required?
>
> - As an additional cleanup, you can change all "should_print(1)" calls to "should_print()" because "1" is the default parameter anyway.
>
> Besides that, your change looks good.
>
> Thank you and best regards,
> Volker
>
> Ran hotspot:tier1 using fastdebug build. Only gtest/GTestWrapper.java failed.
> That's another issue. Currently, Openjdk can't execute any gtest because of a linkage error.
> Error occurred during initialization of VM
> Unable to load native library: /backup/jdk/build/linux-x86_64-server-fastdebug/images/jdk/lib/libjava.so: symbol JVM_GetPermittedSubclasses version SUNWprivate_1.1 not defined in file libjvm.so with link time reference
>
> Thanks,
> --lx
>
>
>
>
>
More information about the hotspot-compiler-dev
mailing list