[9] RFR (XS): 8153340: Incorrect lower bound for AllocatePrefetchDistance with AllocatePrefetchStyle=3

[Vladimir Kozlov]

On 4/26/16 4:56 AM, Zoltán Majó wrote:
> Hi Vladimir,
>
>
> thank you for the feedback! Please see comments below.
>
> On 04/22/2016 12:37 AM, Vladimir Kozlov wrote:
>> Hi, Zoltan
>>
>> I think we should change code in prefetch_allocation() instead:
>>
>>       Node *cache_adr = new AddPNode(old_eden_top, old_eden_top,
>> _igvn.MakeConX(step_size + distance));
>
> The problem is that AllocatePrefetchStyle must align the first prefetched address to 8 bytes, otherwise the emitted stxa
> instruction could cause a SIGBUS. But the alignment does not have to be at step_size boundary, 8-byte alignment is
> sufficient.

Actually it has to be step_size (cache line size) aligned - BIS instruction is triggered when the address of stxa is the 
beginning of cache line for AllocatePrefetchStyle == 3. If it is just 8 bytes aligned it will be simple store.
We should require AllocatePrefetchStepSize to be 8 bytes aligned in vm_version_sparc.cpp instead of:
+       // BIS instructions require 8-byte aligned addresses
+       Node* mask = _igvn.MakeConX(~(intptr_t)(wordSize - 1));


>
>>
>> These way we allow AllocatePrefetchDistance == 0 in all AllocatePrefetchStyle cases - it is consistent.
>
> Unfortunately, it is not easy to have the same limit for AllocatePrefetchDistance on all platforms. Due to the 8-byte
> alignment performed by compiled code, the lower limit of 8 for AllocatePrefetchDistance is needed on SPARC; the lower
> limit of 0 works fine on all other platforms.
>
> I've started looking at the consistency of flags controlling allocation prefetch in general, as we have other issues
> open related to them (e.g., JDK-8151622). We're touching related code now, so I thought, maybe it makes sense to fix all
> remaining issues at once.

Agree. I think AllocatePrefetchStyle=2 is broken on all platforms - it should be used only if UseTLAB is true. Please, look.
And I think Abstract_VM_Version::_reserve_for_allocation_prefetch should be set for all styles on all platforms to avoid 
accessing beyond heap. Prefetch instructions doc say that they does not trap but we should be careful.

>
> The updated webrev does the following (in addition to fixing the original problem with AllocatePrefetchDistance):
>
> 1. Enforce AllocatePrefetchStyle = 3 if AllocatePrefetchInstr = 1 (i.e., BIS instructions are used for prefetching). As
> far as I understand, AllocatePrefetchStyle = 3 was added to support prefetching with BIS, so if BIS is enabled, we
> should use AllocatePrefetchStyle = 3.

Correct - if BIS (AllocatePrefetchInstr = 1) is used we should select AllocatePrefetchStyle = 3.
But we should allow AllocatePrefetchStyle = 3  if normal prefetch instructions (or other platforms) are used.
I think we should update comment in globals.hpp to say "generate one prefetch per cache line" without saying BIS.

But I agree if BIS is not available we should not use BIS AllocatePrefetchInstr = 1.

>
> 2. AllocatePrefetchStyle = 3 is SPARC-specific, so disallow it on non-SPARC platforms.

It could be useful on other platforms since it does one access per cache line.

>
> 3. Enforce that AllocatePrefetchStepSize is multiple of 8 if AllocatePrefetchStyle is 3 (due to alignment requirements).

That is correct since stxa requires at least 8 bytes alignment (as stx).

>
> 3. Determine the number of lines to prefetch in the same way for all prefetch styles:
> lines = (prefecth instance allocation) ? AllocateInstancePrefetchLines : AllocatePrefetchLines

Agree.

>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~zmajo/8153340/webrev.01/

vm_version_sparc.cpp
AllocatePrefetchInstr = 0 should be set for all styles (not only 1) when BIS is not available.

Thanks,
Vladimir

>
> Testing:
> - JPRT (incl. TestOptionsWithRanges)
> - local testing on a SPARC machine.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>
>>
>> Thanks,
>> Vladimir
>>
>> On 4/21/16 4:30 AM, Zoltán Majó wrote:
>>> Hi,
>>>
>>>
>>> please review the patch for 8153340.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8153340
>>>
>>>
>>> Problem: The VM crashes if AllocatePrefetchStyle==3 and AllocatePrefetchDistance==0. The crash happens due to the way
>>> the address for the first prefetch instruction is calculated [1]:
>>>
>>> If distance==0, cache_addr == old_eden_top. Then, cache_adr &= ~(AllocatePrefetchStepSize - 1) which can zero some of
>>> the bits of cache_adr. That result in accesses *before* the newly allocated object.
>>>
>>>
>>> Solution: Set lower limit of AllocatePrefetchDistance to AllocatePrefetchStepSize (for AllocatePrefetchStyle == 3).
>>> Unquarantine test.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~zmajo/8153340/webrev.00/
>>>
>>> Testing:
>>> - JPRT (incl. TestOptionsWithRanges.java)
>>> - local testing on a SPARC machine.
>>>
>>> Thank you!
>>>
>>> Best regards,
>>>
>>>
>>> Zoltan
>>>
>>> [1] http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/f27c00e6f6bf/src/share/vm/opto/macro.cpp#l1941
>