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

[Liu, Xin]

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