RFR: 8337251: C1: Improve Class.isInstance intrinsic [v2]

[Vladimir Ivanov]

On Thu, 5 Dec 2024 17:22:40 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> This replaces a runtime call to `Runtime1::is_instance_of()` by a platform-dependent C1 intrinsic.
>> 
>> This improves overall performance significantly. and it minimizes icache footprint.
>> 
>> The original commit contains this comment:
>> 
>> 
>>   // TODO could try to substitute this node with an equivalent InstanceOf
>>   // if clazz is known to be a constant Class. This will pick up newly found
>>   // constants after HIR construction. I'll leave this to a future change.
>> 
>> 
>> 
>> However, there's little performance to be gained by restricting this optimization to constant Class instances, and after this this patch, C1 `Class.isInstance()` compares favorably with the current platform-dependent `instanceof` intrinsic.
>> 
>> It's not strictly necessary for other platforms to implement this optimization.
>> 
>> Performance:
>> 
>> Xeon-E5 2430, before and after::
>> 
>> 
>> Benchmark                               Score    Error       Score     Error  Units
>> SecondarySupersLookup.testNegative00   11.783 ±  0.491      10.459 ±   0.183  ns/op
>> SecondarySupersLookup.testNegative01   11.757 ±  0.127      10.475 ±   0.661  ns/op
>> SecondarySupersLookup.testNegative02   11.771 ±  0.700      10.479 ±   0.357  ns/op
>> SecondarySupersLookup.testNegative55   23.997 ±  1.816      16.854 ±   1.034  ns/op
>> SecondarySupersLookup.testNegative60   29.598 ±  1.326      26.828 ±   0.637  ns/op
>> SecondarySupersLookup.testNegative63   74.528 ±  3.157      69.431 ±   0.357  ns/op
>> SecondarySupersLookup.testNegative64   75.936 ±  1.805      70.124 ±   0.397  ns/op
>> 
>> SecondarySupersLookup.testPositive01   15.257 ±  1.179       9.722 ±   0.326  ns/op
>> SecondarySupersLookup.testPositive02   15.164 ±  1.383       9.737 ±   0.708  ns/op
>> SecondarySupersLookup.testPositive03   15.166 ±  0.934       9.726 ±   0.184  ns/op
>> SecondarySupersLookup.testPositive40   20.384 ±  0.530      12.805 ±   0.778  ns/op
>> SecondarySupersLookup.testPositive50   15.118 ±  0.140       9.735 ±   0.555  ns/op
>> SecondarySupersLookup.testPositive60   20.415 ±  3.083      11.603 ±   0.106  ns/op
>> SecondarySupersLookup.testPositive63   65.478 ±  8.484      58.507 ±   2.837  ns/op
>> SecondarySupersLookup.testPositive64   75.880 ±  1.047      68.667 ±   1.347  ns/op
>> 
>> 
>> AArch64 (Apple M1)
>> 
>> 
>> Benchmark                              Score   Error   Score   Error  Units
>> SecondarySupersLookup.testNegative00   4.139 ± 0.005   2.815 ± 0.014  ns/op
>> SecondarySupersLookup.testNegative01   4.071 ± 0.153   ...
>
> Andrew Haley has updated the pull request incrementally with seven additional commits since the last revision:
> 
>  - Windows fix, maybe.
>  - Update
>  - Update
>  - Test fix/
>  - Test fix/
>  - Fix test failure
>  - Reorganize C1 is_instance_of stub handling

Overall, looks good.

src/hotspot/share/c1/c1_LIRGenerator.cpp line 1247:

> 1245:   }
> 1246: 
> 1247: #ifdef HAVE_PD_C1_IS_INSTANCE_OF_STUB

Is it the first occurrence of optional stub on C1? Alternatively, you could provide a default implementation on all other platforms which simply calls into `Runtime1::is_instance_of`.

Also, the macro doesn't look pretty here. `c1_Defs_*.hpp` promote a different pattern (a constant). I understand that it requires changing all `c1_Defs_*.hpp` files, but it would definitely look cleaner here.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22491#pullrequestreview-2486257442
PR Review Comment: https://git.openjdk.org/jdk/pull/22491#discussion_r1874217487