On Thu, 26 Sep 2024 17:25:06 GMT, Scott Gibbons <sgibb...@openjdk.org> wrote:
>> @sviswa7 or @asgibbons WDYT about including >> https://github.com/rkennke/jdk/commit/097c2afa04397773e514552dfb942aa889bfa2c1 >> as part of compact object headers implementation? Otherwise we would have >> to disable indexOf intrinsic when running with compact headers, because of >> the assumption that array headers are >= 16 bytes, which is no longer true >> with compact headers. > > @rkennke I reviewed [rkennke@ > 097c2af](https://github.com/rkennke/jdk/commit/097c2afa04397773e514552dfb942aa889bfa2c1) > and the code looks good to me. I would prefer this approach instead of not > generating the IndexOf intrinsic. > > Should the controlling `if` be conditioned on `UseCompactObjectHeaders` > instead of `arrayOopDesc::base_offset_in_bytes`? I can see benefits to > either - which provides more clarity? I like the assert as it makes the > intention clear (thanks!). I like to have the functional connection: if - for whatever reason - the array base offset is smaller than 16, we need to deal with that. The reason for this happens to be `UseCompactObjectHeaders`, but that may not be clear to the reader of the code. I could add an `assert(UseCompactObjectHeaders` in that branch to make that connection clear. Also consider that `UseCompactObjectHeaders` is intended to go away at some point. I wonder if having 2 or 3 branches ahead of the main-loop (which probably doesn't do much, because haystack is <=32 bytes) is a useful approach, or if there may be a better way to get the bytes on the stack? I don't know enough about the implementation to make that judgement. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1778230714
