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

[Zoltán Majó]

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.

>
> 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.

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.

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

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

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

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

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