JDK-8160748 : Inconsistent types for ideal_reg
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 9,10
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2016-07-02
  • Updated: 2021-02-01
  • Resolved: 2017-04-12
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 7 JDK 8 Other
10 b21Fixed 7u221Fixed 8u192Fixed openjdk8u292Fixed
Related Reports
Relates :  
JDK-8257192 - Integrate AArch64 JIT port into 8u
Description
There is an inconsistency about whether register categories used with Node and those used with Type are signed.

- The Node class defines "uint Node::ideal_reg()" and "uint Node::NotAMachineReg" is a special value which "must be > max. machine register".

- The Type class has "int ideal_reg()" which deals in the same values.

This mostly results in some implicit potentially narrowing conversions in various places, due to the chosen value of NotAMachineReg.  However, the initialization of the Type::_type_info array includes some having their ideal_reg member initialized to NotAMachineReg.  Such implicit narrowings are forbidden by C++11.

The invalid narrowings could be worked around via casts.  But that's building in assumptions about the value of NotAMachineReg.  And it seems like it might be better to remove the inconsistency, if possible.

[This is a followup to JDK-8160353.]
Comments
I think a clean (avoiding workaround casts) and practical (not using C++11 features) solution is to change Type::ideal_reg to return uint to be consistent with Node::ideal_reg. I'm testing a changeset that does that, and fixes some existing type mismatches where calls to Node::ideal_reg are being bound to int variables.
05-04-2017
[~ysuenaga] Sorry, I misunderstood your initial comment and webrev pointer. I thought (without looking) that was the initializer cast method, but it's not. Instead, it's a quite thorough reimplementation of the register types using a C++11 enum class. That's interesting, and in some ways quite appealing. However, because it requires a C++11 feature, I think we can't go that route yet. We need to do something in order to support Visual Studio 2015 and later, but I don't think we're yet ready to *require* C++11 support. I'm presently looking at this, as it appears to be the only thing blocking the use of -std=gnu++11 when building with a sufficiently recent version of gcc.
05-04-2017
My webrev resolve the problems as below: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-July/023611.html ------------- /home/ysuenaga/OpenJDK/hs/hotspot/src/share/vm/opto/type.cpp:101:1: error: narrowing conversion of '(uint)Node::NotAMachineReg' from 'uint {aka unsigned int}' to 'int' inside { } [-Werror=narrowing] }; ^ /home/ysuenaga/OpenJDK/hs/hotspot/src/share/vm/opto/type.cpp:101:1: error: narrowing conversion of '(uint)Node::NotAMachineReg' from 'uint {aka unsigned int}' to 'int' inside { } [-Werror=narrowing] /home/ysuenaga/OpenJDK/hs/hotspot/src/share/vm/opto/type.cpp:101:1: error: narrowing conversion of '(uint)Node::NotAMachineReg' from 'uint {aka unsigned int}' to 'int' inside { } [-Werror=narrowing] cc1plus: all warnings being treated as errors ------------- In case of type.cpp, I think the cast of webrev is safe because Node::NotAMachineReg can be represented as int.
14-02-2017
[~ysuenaga] Your solution was the workaround casts mentioned in the description, correct? That's what I see here: http://cr.openjdk.java.net/~ysuenaga/jdk9-for-gcc6/hotspot/src/share/vm/opto/type.cpp.frames.html. Such casts are a bandaid to cover a deeper problem. And such casts will also cause problems for fixing JDK-8135181. That's why I opened this bug rather than accepting your cast patch.
14-02-2017
Thank you!
14-02-2017
Yes. We will pass the build process when you apply this patch. However, I have not checked it with current (tip of jdk9 and jdk10) repos.
14-02-2017
Thank you, [~ysuenaga]. Just that I understand better: you built the VM with a C++11-enabled gcc version on x86_64 and the build failed (before your patch) and passed after your patch was applied?
14-02-2017
My webrev has been checked on x86_64 Linux. So you need to check it on other platforms.
14-02-2017
Kim Barrett [~kbarrett] pointed out to me that this issue might be a blocker for building the VM with Visual Studio 2015 or later. It can be also a blocker for turning on C++11 for gcc and others. I'm not sure if we are currently building the VM that way, but once we do, it might be good to reclassify this enhancement as a bug.
14-02-2017
I clarified all points which we should fix with C++11 scoped enum: http://cr.openjdk.java.net/~ysuenaga/JDK-8160748/webrev.01/
11-07-2016