On Thu, 23 May 2024 19:02:05 GMT, Daniel Jeliński <djelin...@openjdk.org> wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix for IndexOf.java on mac > > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 268: > >> 266: __ cmpq(needle_len_p, 0); >> 267: __ jg_b(L_nextCheck); >> 268: __ xorq(rax, rax); > > out of curiosity, is there any advantage to using `xorq` instead of `xorl` > here? > > https://stackoverflow.com/a/33668295/7707617 suggests that `xorl` might be > better, but it's a bit dated now. Thanks for finding this. It was ignorance on my part as I thought the xorq would have logic to not emit the REX prefix if not necessary, but it doesn't. Fixed. > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 449: > >> 447: __ cmpq(r13, NUMBER_OF_CASES - 1); >> 448: __ ja(L_smallCaseDefault); >> 449: __ mov64(r15, (int64_t)small_jump_table); > > would it make sense to use `lea` here? It may, but I believe the movq is shorter (although maybe not to r15). I'll do some experimentation. > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 803: > >> 801: __ movq(index, needle_len); >> 802: __ andq(index, 0xf); // nLen % 16 >> 803: __ movq(offset, 0x10); > > `movl` or `movptr` would produce a shorter encoding I tried to be consistent with the whole {q,l} syntax throughout when referring to each symbolic register. I feel that changing this would ripple through the code. @sviswa7 what do you think? > src/hotspot/cpu/x86/stubGenerator_x86_64_string.cpp line 1544: > >> 1542: } >> 1543: >> 1544: __ align(8); > > why `8` and not `OptoLoopAlignment` ? Short answer - because I didn't know there was such a thing as `OptoLoopAlignment`. I'll change that throughout at the top of my loops. Thanks. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612201503 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612207461 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612216483 PR Review Comment: https://git.openjdk.org/jdk/pull/16753#discussion_r1612218363
