RFR: 8339220: [s390x] TestIfMinMax.java failure [v2]

[Lutz Schmidt]

On Thu, 26 Sep 2024 10:06:08 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> This test enables Conditional moves for long operands for s390x. Which fixes the test-case.
>> 
>> Ran tier1 and not saw any regression.
>
> Amit Kumar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   suggestions from Andrew

Changes requested by lucy (Reviewer).

src/hotspot/cpu/s390/matcher_s390.hpp line 69:

> 67:   static int long_cmove_cost() {
> 68:     // z196/z11 or later hardware support conditional moves
> 69:     return VM_Version::get_model_index() >= 5 ? 0 : ConditionalMoveLimit;

Why didn't you use `has_LoadStoreConditional()`? This method uses the facility indication flag and does not rely on some artificial architecture generation detection. At least in theory, it could happen that the facility you are testing for was not installed or disabled on the machine you are running on.

src/hotspot/cpu/s390/matcher_s390.hpp line 74:

> 72:   static int float_cmove_cost() {
> 73:     // z196/z11 or later hardware support conditional moves
> 74:     return VM_Version::get_model_index() >= 5 ? 0 : ConditionalMoveLimit;

Same as above.

src/hotspot/cpu/s390/vm_version_s390.hpp line 174:

> 172:  public:
> 173: 
> 174:   static int  get_model_index();

With the above, this method can become private again.

src/hotspot/cpu/x86/vm_version_x86.hpp line 656:

> 654:   //       4   -  486
> 655:   //       5   -  Pentium
> 656:   //       6   -  PentiumPro, Pentium II, Celer;on, Xeon, Pentium III, Athlon,

I haven't heard of that processor family. Nor does intel talk about it: https://www.intel.com/content/www/us/en/products/details/processors/celeron.html

-------------

PR Review: https://git.openjdk.org/jdk/pull/21198#pullrequestreview-2330810035
PR Review Comment: https://git.openjdk.org/jdk/pull/21198#discussion_r1776807146
PR Review Comment: https://git.openjdk.org/jdk/pull/21198#discussion_r1776807467
PR Review Comment: https://git.openjdk.org/jdk/pull/21198#discussion_r1776808287
PR Review Comment: https://git.openjdk.org/jdk/pull/21198#discussion_r1776810653