8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands

[chhagedorn]

When using a compiler directives file with PrintIdealPhase:

[
  {
    match : "Test::*",
    log : true,
    PrintIdealPhase : "BEFORE_MATCHING"
  }
]

together with other compile commands specified in compilerdirectives_common_flags and/or compilerdirectives_c2_flags:

jdk/src/hotspot/share/compiler/compilerDirectives.hpp

Lines 38 to 39 in aff5ff1

	#define compilerdirectives_common_flags(cflags) \
	cflags(Enable, bool, false, Unknown) \

jdk/src/hotspot/share/compiler/compilerDirectives.hpp

Lines 63 to 64 in aff5ff1

	#define compilerdirectives_c2_flags(cflags) \
	cflags(BlockLayoutByFrequency, bool, BlockLayoutByFrequency, BlockLayoutByFrequency) \

then the PrintIdealPhase option is ignored.

The reason is that when cloning the DirectiveSet for the current compilation in DirectiveSet::clone(), we only set PrintIdealPhaseOption but forget to also set _ideal_phase_name_mask which is used when deciding if a compile phase should be dumped or not. As a result, the mask keeps its default value zero and nothing is dumped because Compile::shoud_print_phase() returns false:

jdk/src/hotspot/share/opto/compile.cpp

Lines 5060 to 5067 in aff5ff1

	bool Compile::should_print_phase(CompilerPhaseType cpt) {
	#ifndef PRODUCT
	if ((_directive->ideal_phase_mask() & CompilerPhaseTypeHelper::to_bitmask(cpt)) != 0) {
	return true;
	}
	#endif
	return false;
	}

The fix is to also clone the old value of _ideal_phase_name_mask.

Thanks,
Christian

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands

Reviewers

• Roberto Castañeda Lozano (@robcasloz - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10283/head:pull/10283
$ git checkout pull/10283

Update a local copy of the PR:
$ git checkout pull/10283
$ git pull https://git.openjdk.org/jdk pull/10283/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10283

View PR using the GUI difftool:
$ git pr show -t 10283

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10283.diff

[bridgekeeper]

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@chhagedorn The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (87e97895)

[robcasloz]

Looks good to me! Besides reproducing the reported issue, I also tested that

• compiler directives take precedence over compile commands (as specified in the Compiler Control JEP, paragraph "CompileCommand and backwards compatibility") if both provide a PrintIdealPhase; and
• the fix makes it possible to run compile commands on top of IR tests in your prototype changeset for JDK-8280378.

[openjdk]

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands

Reviewed-by: rcastanedalo, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 62 new commits pushed to the master branch:

• 4b297c1: 8293892: Add links to JVMS 19 and 20 from ClassFileFormatVersion enum constants
• dfb9c06: 8293535: jdk/javadoc/doclet/testJavaFX/TestJavaFxMode.java fail with jfx
• f42caef: 8293550: Optionally add get-task-allow entitlement to macos binaries
• 5feca68: 8293840: RISC-V: Remove cbuf parameter from far_call/far_jump/trampoline_call
• 39cd163: 8293578: Duplicate ldc generated by javac
• 7765942: 8290367: Update default value and extend the scope of com.sun.jndi.ldap.object.trustSerialData system property
• 11e7d53: 8293819: sun/util/logging/PlatformLoggerTest.java failed with "RuntimeException: Retrieved backing PlatformLogger level null is not the expected CONFIG"
• 141d5f5: 8293767: AWT test TestSinhalaChar.java has old SCCS markings
• 3beca2d: 8291600: [vectorapi] vector cast op check is not always needed for vector mask cast
• 9a40b76: 8293842: IPv6-only systems throws UnsupportedOperationException for several socket/TCP options
• ... and 52 more: https://git.openjdk.org/jdk/compare/37df5f56259429482cfdbe38e8b6256f1efaf9e8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[chhagedorn]

Thanks a lot Roberto for your review, the additional testing, and originally finding this bug!

[vnkozlov]

Good.

[chhagedorn]

Thanks Vladimir for your review!

/integrate

[openjdk]

Going to push as commit d41f69f.
Since your change was applied there have been 79 commits pushed to the master branch:

• 471e2f1: 8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue
• a93cf92: 8293920: G1: Add index based heap region iteration
• 36c9034: 8293808: mscapi destroyKeyContainer enhance KeyStoreException: Access is denied exception
• cbd0688: 8293851: hs_err should print more stack in hex dump
• 04d7b7d: 8293503: gc/metaspace/TestMetaspacePerfCounters.java#Epsilon-64 failed assertGreaterThanOrEqual: expected MMM >= NNN
• d77c464: 8293891: gc/g1/mixedgc/TestOldGenCollectionUsage.java (still) assumes that GCs take 1ms minimum
• d7c1a76: 8293861: G1: Disable preventive GCs by default
• 43f7f47: 8293499: Provide jmod --compress option
• 26e08cf: 8293844: C2: Verify Location::{oop,normal} types in PhaseOutput::FillLocArray
• 357a2cc: 8293937: x86: Drop LP64 conditions from clearly x86_32 code
• ... and 69 more: https://git.openjdk.org/jdk/compare/37df5f56259429482cfdbe38e8b6256f1efaf9e8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit d41f69f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.