On Fri, 31 Jan 2025 10:31:48 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:
>> Johannes Graham has updated the pull request incrementally with one additional commit since the last revision:
>>
>> move template def to header
>
> src/hotspot/share/opto/addnode.hpp line 257:
>
>> 255:
>> 256: template<class S, class U>
>> 257: static S calc_xor_max(const S hi_0, const S hi_1) {
>
> If this is exposed in the header file, then I think it would be better to have it be a static function of a class to avoid convoluting the global namespace. Suggestion: Can you have `XorINode::calc_max` which will call a static `calc_xor_max` inside the cpp file.
I've reorganized it.
> test/hotspot/gtest/opto/test_xor_node.cpp line 50:
>
>> 48:
>> 49: template <class S>
>> 50: void test_exhaustive_values(S hi_0, S hi_1){
>
> You may want to test exhaustively for the `hi` values here, too. E.g:
>
> void test_exhaustive(S limit) {
> for (S hi0 = 0; hi0 <= limit; hi0++) {
> for (S hi1 = 0; hi1 <= limit; hi1++) {
> S max = calc_max(hi0, hi1);
> for (S v0 = 0; v0 <= hi0; v0++) {
> for (S v1 = 0; v1 <= hi1; v1++) {
> S v = v0 | v1;
> EXPECT_LE(v, max);
> }
> }
> }
> }
> }
Was my intention that the test should have been doing that. I've cleaned it up to clarify it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1938160654
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1938160433