RFR(L): JEP165: Compiler Control

[Nils Eliasson]

Hi Roland,

On 2015-09-23 10:14, Roland Westrelin wrote:
>> Hotspot webrev: http://cr.openjdk.java.net/~neliasso/8046155/webrev.01/
> compileBroker.cpp
>
>   210   /*CompilerDirectives* _default_directives = new CompilerDirectives();
>   211   char str[] = "*.*";
>   212   const char* error_msg = NULL;
>   213   _default_directives->add_match(str, error_msg);
>   214   assert(error_msg == NULL, "Must succeed.");
>   215   CompileBroker::dirstack()->push(_default_directives);*/
>
> should go away.
Yes.
>
> c1_LIRGenerator.cpp
>
> Why do you make those changes? They are breaking tiered compilation AFAICT.
Typo from a bad revert. Fixing.
> compileBroker.cpp
>
> What does that do?

Checking if a method can be compiled. Similar to the 
CompilerOracle::should_exclude. I'll update the comments.
>
> 1163   // Breaking the abstraction - directives are only used inside a compilation otherwise.
> 1164   DirectiveSet* dirset = dirstack()->getMatchingDirective(method, comp_level);
> 1165   bool excluded = !dirset->EnabledOption;
> 1166   {
> 1167     MutexLockerEx locker(DirectivesStack_lock, Mutex::_no_safepoint_check_flag);
> 1168     dirset->directive()->dec_refcount();
> 1169   }
>
> block.hpp
>
>   371   // ciEnv for retrieving flags
>   372   ciEnv* _ciEnv;
>
> Where are you using that one? There’s usually a reference to Compile around and that’s how we get to the ciEnv.

ciEnv have the nice properties to be the same regardless of compiler, 
and to have a life time of a compilation. But I have moved the 
directiveSet to the Compile instead - it makes the accessor code shorter 
and nicer, but unfortunately duplicates code in (Opto/)Compile, 
(C1/)Compilation and shark compiler.

>
> c2_globals.hpp
>
>   150   diagnostic(bool, PrintOptoAssembly, false,                                \
>   151           "Print New compiler assembly output")                             \
>
>   591   diagnostic(bool, TraceSpilling, false,                                    \
>   592           "Trace spilling")                                                 \
>
> Why are those needed? It doesn’t feel right that you have to change the scope of an option for that change.
Those two where mirrored from CompileCommand options. The option 
commands are stringly typed and never bothered checking for scope or 
type. To have backwards compatibility I changed the flag definitions, 
but kept the code behind debug. I can solve it an another way if you like.
>
> CompilerQueueTest.java
>
> Why that change?
Left over from when I split the change. Removing.
> nmethod.cpp
>
> 508     if (Thread::current()->is_Compiler_thread()) {
> native methods are not generated by a compiler thread so that wouldn’t work.
Removed the code and moved the PrintOptoAssembly up the call hierarchy.
>
> In general, where are command line options (like PrintInlining for instance) recorded with that framework so their value can later be queried:
>
> if (!compilation()->env()->dirset()->PrintInliningOption) {
>
> ?

They are set from the directivesParser.cpp/hpp - check the key list that 
maps names to types and fields in the directive: const 
DirectivesParser::key DirectivesParser::keys[]

The setters are defined in compilerDirectives.cpp:
   // Casting to get the same function signature for all setters. Used 
from parser.
   #define set_function_definition(name, type, dvalue, cc_flag) void 
set_##name(void* value) { type val = *(type*)value; name##Option = val; 
_modified[name##Index] = 1; }



New webrev coming up,

Thanks for taking a look!
//Nils

>
> Roland.