On Fri, 27 Sep 2024 08:24:50 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> @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. I believe the code in the patch is good enough as-is, especially if `UseCompactObjectHeaders` is slated to go away. The existing `if` will prevent the < 16 byte header code from being emitted, which is the desired behavior - i.e., if the header size is >= 16, there will be no code emitted to the intrinsic for that block. So there will not be an additional branch for the code when it is executed. I'm good with a comment tying `UseCompactObjectHeaders` to the condition. The comment can be removed when the flag is removed. "Ship it" :-) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1778739517
