On Thu, 12 May 2022 22:40:50 GMT, Vladimir Ivanov <vliva...@openjdk.org> wrote:
>> Jatin Bhateja has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 11 commits: >> >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - 8284960: Correcting a typo. >> - 8284960: Integrating changes from panama-vector (Add @since 19 tags). >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - 8284960: AARCH64 backend changes. >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 >> - ... and 1 more: >> https://git.openjdk.java.net/jdk/compare/3fa1c404...b021e082 > > src/hotspot/cpu/x86/matcher_x86.hpp line 195: > >> 193: case Op_PopCountVI: >> 194: return ((ety == T_INT && >> VM_Version::supports_avx512_vpopcntdq()) || >> 195: (is_subword_type(ety) && >> VM_Version::supports_avx512_bitalg())) ? 0 : 50; > > Should be easier to read when the condition is split. E.g.: > > if (is_subword_type(ety)) { > return VM_Version::supports_avx512_bitalg())) ? 0 : 50; > } else { > assert(ety == T_INT, "sanity"); // for documentation purposes > return VM_Version::supports_avx512_vpopcntdq() ? 0 : 50; > } DONE > src/hotspot/cpu/x86/vm_version_x86.hpp line 375: > >> 373: decl(RDTSCP, "rdtscp", 48) /* RDTSCP >> instruction */ \ >> 374: decl(RDPID, "rdpid", 49) /* RDPID >> instruction */ \ >> 375: decl(FSRM, "fsrm", 50) /* Fast Short REP >> MOV */ \ > > `test/lib-test/jdk/test/whitebox/CPUInfoTest.java` should be adjusted as > well, shouldn't it? Yes, test updated appropriately. > src/hotspot/share/classfile/vmIntrinsics.hpp line 1152: > >> 1150: >> "Ljdk/internal/vm/vector/VectorSupport$ComExpOperation;)" >> \ >> 1151: >> "Ljdk/internal/vm/vector/VectorSupport$VectorPayload;") >> \ >> 1152: do_name(vector_comexp_op_name, "comExpOp") >> \ > > I don't see much value in trying to shorten the name by abbreviating it. I > find it easier to read in an expanded form: > ` compressExpandOp`, `vector_compress_expand_op_name`, > `_VectorCompressExpand`, etc. DONE > src/hotspot/share/opto/c2compiler.cpp line 521: > >> 519: if (!Matcher::match_rule_supported(Op_SignumF)) return false; >> 520: break; >> 521: case vmIntrinsics::_VectorComExp: > > Why `_VectorComExp` intrinsic is special? Other vector intrinsics are handled > later and in a different manner. > > What about `ExpandV` case? It was an attempt to facilitate in-lining of these APIs over targets which do not intrinsify them. I agree its not a generic fix since three APIs are piggybacking on same entry point and without the knowledge of opcode it will be inappropriate to take any call at this place, lazy intrinsification gives opportunity for some of the predications to concertize as compilation happens under closed world assumptions. > src/hotspot/share/opto/compile.cpp line 3416: > >> 3414: >> 3415: case Op_ReverseBytesV: >> 3416: case Op_ReverseV: { > > Can you elaborate, please, why it is performed so late in the optimization > phase (at the very end during graph reshaping) and not during GVN? Its more of a chicken-egg problem here, for masked reverse operation, Reverse IR node is followed by a Blend Node, thus in such a case doing an eager Identity transform in Reverse::Identity will not work, also deferring this to blend may also not work since it could be a non-masked reverse operation, we could have handled it as a special case in inline_vector_nary_operation, but handling such special case in final graph reshaping looked more appropriate. https://github.com/openjdk/panama-vector/pull/182#discussion_r845678080 ------------- PR: https://git.openjdk.java.net/jdk/pull/8425