Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8265403: consolidate definition of CPU features #3558

Closed
wants to merge 4 commits into from

Conversation

@dougxc
Copy link
Member

@dougxc dougxc commented Apr 17, 2021

While porting JDK-8224974 to Graal, I noticed that new CPU features were defined for x86 and AArch64 without being exposed via JVMCI. To avoid this problem in future, this PR updates x86 and AArch64 to define CPU features with a single macro that is used to generate enum declarations as well as vmstructs entries.

In addition, the JVMCI API is updated to exposes the new CPU feature constants and now has a check that ensure these constants are in sync with the underlying macro definition.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3558/head:pull/3558
$ git checkout pull/3558

Update a local copy of the PR:
$ git checkout pull/3558
$ git pull https://git.openjdk.java.net/jdk pull/3558/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3558

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3558.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 17, 2021

👋 Welcome back dnsimon! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 17, 2021

@dougxc The following labels will be automatically applied to this pull request:

  • build
  • hotspot

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

Loading

}

Map<String, Long> constants = config.getStore().getConstants();
Function<String, CPUFeature> nameToFeature = name -> name.equals("3DNOW_PREFETCH") ? CPUFeature.AMD_3DNOW_PREFETCH : CPUFeature.valueOf(name);
Copy link
Member Author

@dougxc dougxc Apr 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AMD_3DNOW_PREFETCH enum constant has to keep its old name to preserve backward compatibility.

Loading

if (_features & CPU_LSE) strcat(buf, ", lse");
if (_features & CPU_SVE) strcat(buf, ", sve");
if (_features & CPU_SVE2) strcat(buf, ", sve2");
#define ADD_FEATURE_IF_SUPPORTED(id, name, bit) if (_features & CPU_##id) strcat(buf, ", " name);
Copy link
Member Author

@dougxc dougxc Apr 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why only some of the supported AArch64 CPU features were being added to _features_string but I assume there's no harm in adding them all.

Loading

static const char* _features_names[];

// NB! When adding new CPU feature detection consider updating vmStructs_x86.hpp, vmStructs_jvmci.hpp, and VM_Version::get_processor_features().
Copy link
Member Author

@dougxc dougxc Apr 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this comment any more as the derivative declarations are now automatically kept up to date.

Loading

@dougxc dougxc marked this pull request as ready for review Apr 18, 2021
@openjdk openjdk bot added the rfr label Apr 18, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 18, 2021

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 19, 2021

/label add hotspot-compiler
/label add hotspot-runtime

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@vnkozlov
The hotspot-compiler label was successfully added.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 19, 2021

/label add hotspot-runtime

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@vnkozlov
The hotspot-runtime label was successfully added.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 19, 2021

/label remove hotspot

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@vnkozlov The hotspot-runtime label was already applied.

Loading

@openjdk openjdk bot removed the hotspot label Apr 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@vnkozlov
The hotspot label was successfully removed.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Please, update copyright years in files you touched.

Loading

decl(AVX512_VBMI2, "avx512_vbmi2", 44) /* VBMI2 shift left double instructions */ \
decl(AVX512_VBMI, "avx512_vbmi", 45) /* Vector BMI instructions */ \
decl(HV, "hv", 46) /* Hypervisor instructions */
#define DECLARE_CPU_FEATURE_FLAG(id, name, bit) CPU_##id = (1ULL << bit),
Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty line before it to separate CPU_FEATURE_FLAGS macro more clear.

Loading

decl(SVE2, "sve2", 28) \
decl(STXR_PREFETCH, "stxr_prefetch", 29) \
decl(A53MAC, "a53mac", 30)
#define DECLARE_CPU_FEATURE_FLAG(id, name, bit) CPU_##id = (1 << bit),
Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty line before to separate CPU_FEATURE_FLAGS macro.

Loading

Copy link
Member Author

@dougxc dougxc Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

Loading

#define VM_LONG_CONSTANTS_CPU(declare_constant, declare_preprocessor_constant, declare_c1_constant, declare_c2_constant, declare_c2_preprocessor_constant)

#define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit) GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
#define VM_LONG_CPU_FEATURE_CONSTANTS CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to keep VM_LONG_CONSTANTS_CPU after you removed its body here and in vmStructs_jvmci.cpp?

What about VM_INT_CONSTANTS_CPU here? vmStructs_jvmci.cpp duplicates it.

Loading

Copy link
Member Author

@dougxc dougxc Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vmStructs.cpp and vmStructs_jvmci.cpp are disjoint. This file (i.e. vmStructs_x86.hpp) is only used by vmStructs.cpp.
vmStructs.cpp expects all macros such as VM_LONG_CONSTANTS_CPU to be defined.
vmStructs_jvmci.cpp will provide a dummy definition for missing macros.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Even so they are empty everywhere :(

Loading

declare_constant(VM_Version::CPU_STXR_PREFETCH) \
declare_constant(VM_Version::CPU_A53MAC)
#define DECLARE_INT_CPU_FEATURE_CONSTANT(id, name, bit) GENERATE_VM_INT_CONSTANT_ENTRY(VM_Version::CPU_##id)
#define VM_INT_CPU_FEATURE_CONSTANTS CPU_FEATURE_FLAGS(DECLARE_INT_CPU_FEATURE_CONSTANT)
Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing #undef DECLARE_INT_CPU_FEATURE_CONSTANT.

Loading

Copy link
Member Author

@dougxc dougxc Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it must stay defined up to the point VM_INT_CPU_FEATURE_CONSTANTS is used. Since this is a .cpp file, it's ok to leave it defined.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

Loading

declare_constant(VM_Version::CPU_AVX512_VBMI) \
declare_constant(VM_Version::CPU_HV)
#define DECLARE_LONG_CPU_FEATURE_CONSTANT(id, name, bit) GENERATE_VM_LONG_CONSTANT_ENTRY(VM_Version::CPU_##id)
#define VM_LONG_CPU_FEATURE_CONSTANTS CPU_FEATURE_FLAGS(DECLARE_LONG_CPU_FEATURE_CONSTANT)
Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing #undef DECLARE_LONG_CPU_FEATURE_CONSTANT.

Loading

Copy link
Member Author

@dougxc dougxc Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as for DECLARE_INT_CPU_FEATURE_CONSTANT.

Loading

for (Entry<String, Long> e : constants.entrySet()) {
long bitMask = e.getValue();
String key = e.getKey();
if (key.startsWith("VM_Version::CPU_")) {
Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand this code, it goes over constants values passed from VM and Trying to map them to enumType. It catches cases when a value is missing in enumType. What about case when enumType has extra value which is not defined in constants?

Loading

Copy link
Member Author

@dougxc dougxc Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could warn about that but cannot remove it without breaking backwards capability for JVMCI wrt Graal. Such a deleted capability will simply be seen as "not supported" by Graal.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

You need review from Runtime group too.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 19, 2021

@dougxc 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:

8265403: consolidate definition of CPU features

Reviewed-by: kvn, iklam

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 105 new commits pushed to the master branch:

  • 8e31229: 8265327: Remove check_safepoint_and_suspend_for_native_trans()
  • c9b70c8: 8265240: runtime/Thread/SuspendAtExit.java needs updating
  • 891f72f: 8265606: Reduce allocations in AdapterHandlerLibrary::get_adapter
  • bfc1cd8: 8264320: ShouldNotReachHere in Compile::print_inlining_move_to()
  • 5db64c3: 8265268: Unify ReservedSpace reservation code in initialize and try_reserve_heap
  • 191f1fc: 8265759: Shenandoah: Avoid race for referent in assert
  • b3a319c: 8264842: IGV: different nodes sharing idx are treated as equal
  • 95f0fd6: 8265696: Move CDS sources to src/hotspot/shared/cds
  • a715b09: 8265798: Minimal build broken by JDK-8261090
  • 13d3263: 8265484: Fix up TRAPS usage in GenerateOopMap::compute_map and callers
  • ... and 95 more: https://git.openjdk.java.net/jdk/compare/cb8394a841023b2cf4a9103f47f50e3fd28c5b04...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.

Loading

@openjdk openjdk bot added the ready label Apr 19, 2021
iklam
iklam approved these changes Apr 23, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

Loading

@dougxc
Copy link
Member Author

@dougxc dougxc commented Apr 23, 2021

Thanks for the reviews @vnkozlov and @iklam.

Loading

@dougxc
Copy link
Member Author

@dougxc dougxc commented Apr 23, 2021

/integrate

Loading

@openjdk openjdk bot closed this Apr 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 23, 2021

@dougxc Since your change was applied there have been 112 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 5aed446.

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

Loading

@dougxc dougxc deleted the JDK-8265403 branch May 2, 2021
@dougxc dougxc restored the JDK-8265403 branch May 2, 2021
@dougxc dougxc deleted the JDK-8265403 branch May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants