RFR: 8339220: [s390x] TestIfMinMax.java failure [v2]
Lutz Schmidt
lucy at openjdk.org
Thu Sep 26 10:41:38 UTC 2024
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
More information about the hotspot-dev
mailing list