RFR(S): 8139046: Compiler Control: IVGPrintLevel directive should set PrintIdealGraph

[Hohensee, Paul]

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