RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen

[Paul Sandoz]

On Tue, 26 Jan 2021 13:24:57 GMT, Jie Fu <jiefu at openjdk.org> wrote:

>> The intrinsic enables C2 to more reliably elide bounds checks. I don't know the exact details but at a high-level it transforms signed values into unsigned values (and therefore the signed comparisons become unsigned comparisons), which helps C2 remove checks when there is a dominating check of, for example, an upper loop bound.
>> 
>> You say "the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements". Can you present some examples of Java code and the corresponding C2 generated assembler where this happens?
>
>> The intrinsic enables C2 to more reliably elide bounds checks. I don't know the exact details but at a high-level it transforms signed values into unsigned values (and therefore the signed comparisons become unsigned comparisons), which helps C2 remove checks when there is a dominating check of, for example, an upper loop bound.
>> 
>> You say "the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements". Can you present some examples of Java code and the corresponding C2 generated assembler where this happens?
> 
> Hi @PaulSandoz ,
> 
> I agree with you that let's keep the code as it is for the sake of performance.
> 
> I spent some time looking into the assembly code generated by Objects.checkIndex and inlined if-statements.
> Here are the test program [1], running script [2] and diff [3].
> 
>  - For testSimple [4] that I checked last week, inlined if-statements [5] is better than Objects.checkIndex [6].
>  - However, for testLoop [7], Objects.checkIndex [8] is better than inlined if-statements [9].
>    (I'm sorry I didn't check loop methods last week.)
>    AFAICS, the inlined if-statements will generate two more instructions [10] to check wether idx >= 0 for each loop iteration.
> 
> It seems that the intrinsified Objects.checkIndex will be able to optimize out the lower bound check for loops.
> So I also agree with you that an intrinsified method seems the right choice.
> 
> Thanks.
> Best regards,
> Jie
> 
> [1] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java
> [2] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.sh
> [3] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.diff
> [4] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L10
> [5] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testSimple.log
> [6] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testSimple.log
> [7] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L15
> [8] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testLoop.log
> [9] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log
> [10] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log#L135

Hi Jie,

Thanks for the detailed analysis.

I suspect the difference outside of loops is because of expression "length - (vlen - 1)”.

We need to make intrinsic Objects.checkFromIndexSize. Is that something you might be interested in pursuing?

Paul.

On Jan 26, 2021, at 5:25 AM, Jie Fu <notifications at github.com<mailto:notifications at github.com>> wrote:



The intrinsic enables C2 to more reliably elide bounds checks. I don't know the exact details but at a high-level it transforms signed values into unsigned values (and therefore the signed comparisons become unsigned comparisons), which helps C2 remove checks when there is a dominating check of, for example, an upper loop bound.

You say "the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements". Can you present some examples of Java code and the corresponding C2 generated assembler where this happens?

Hi @PaulSandoz<https://urldefense.com/v3/__https://github.com/PaulSandoz__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_Fqki3MpUu-w$> ,

I agree with you that let's keep the code as it is for the sake of performance.

I spent some time looking into the assembly code generated by Objects.checkIndex and inlined if-statements.
Here are the test program [1], running script [2] and diff [3].

  *   For testSimple [4] that I checked last week, inlined if-statements [5] is better than Objects.checkIndex [6].
  *   However, for testLoop [7], Objects.checkIndex [8] is better than inlined if-statements [9].
(I'm sorry I didn't check loop methods last week.)
AFAICS, the inlined if-statements will generate two more instructions [10] to check wether idx >= 0 for each loop iteration.

It seems that the intrinsified Objects.checkIndex will be able to optimize out the lower bound check for loops.
So I also agree with you that an intrinsified method seems the right choice.

Thanks.
Best regards,
Jie

[1] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_Fqkgg0xOSvw$>
[2] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.sh<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.sh__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkhuQpNMig$>
[3] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.diff<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.diff__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkhABlSb0g$>
[4] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L10<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java*L10__;Iw!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkjdFLWTPg$>
[5] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testSimple.log<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testSimple.log__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkglteeJyA$>
[6] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testSimple.log<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testSimple.log__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_Fqkj7_CEbEw$>
[7] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L15<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java*L15__;Iw!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_Fqkhxo3sJjg$>
[8] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testLoop.log<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testLoop.log__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkhIy4Qh-A$>
[9] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_Fqkh7uhCIiQ$>
[10] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log#L135<https://urldefense.com/v3/__https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log*L135__;Iw!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkiamkUoew$>

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/2127*issuecomment-767538738__;Iw!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_Fqkh5OZMwFg$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AABHQQHPG3FX4YWS6L3L7NTS327DTANCNFSM4WHJ6CHQ__;!!GqivPVa7Brio!MjVcmwqonN75cKLtklQpW6WVz_c3e2SZku7ErC-dlU0rSZ46SKCWqQ_FqkjBYJBa4A$>.

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

PR: https://git.openjdk.java.net/jdk/pull/2127