RFR(S): 8139046: Compiler Control: IVGPrintLevel directive should set PrintIdealGraph
Hohensee, Paul
hohensee at amazon.com
Tue Jun 9 14:36:32 UTC 2020
Looks good for my part.
Thanks,
Paul
On 6/8/20, 3:59 PM, "Liu, Xin" <xxinliu at amazon.com> 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