RFR(S): 8136473: failed: no mismatched stores, except on raw memory: StoreB StoreI
Roland Westrelin
roland.westrelin at oracle.com
Mon Oct 19 15:07:17 UTC 2015
Thanks for the review, Vladimir but Tobias noticed I missed one store in LibraryCallKit::inline_unsafe_access(). Here is a new webrev:
http://cr.openjdk.java.net/~roland/8136473/webrev.04/
With the extra change below.
Roland.
diff --git a/src/share/vm/opto/idealKit.cpp b/src/share/vm/opto/idealKit.cpp
--- a/src/share/vm/opto/idealKit.cpp
+++ b/src/share/vm/opto/idealKit.cpp
@@ -368,7 +368,8 @@
Node* IdealKit::store(Node* ctl, Node* adr, Node *val, BasicType bt,
int adr_idx,
- MemNode::MemOrd mo, bool require_atomic_access) {
+ MemNode::MemOrd mo, bool require_atomic_access,
+ bool mismatched) {
assert(adr_idx != Compile::AliasIdxTop, "use other store_to_memory factory");
const TypePtr* adr_type = NULL;
debug_only(adr_type = C->get_adr_type(adr_idx));
@@ -379,6 +380,9 @@
} else {
st = StoreNode::make(_gvn, ctl, mem, adr, adr_type, val, bt, mo);
}
+ if (mismatched) {
+ st->as_Store()->set_mismatched_access();
+ }
st = transform(st);
set_memory(st, adr_idx);
diff --git a/src/share/vm/opto/idealKit.hpp b/src/share/vm/opto/idealKit.hpp
--- a/src/share/vm/opto/idealKit.hpp
+++ b/src/share/vm/opto/idealKit.hpp
@@ -228,7 +228,9 @@
BasicType bt,
int adr_idx,
MemNode::MemOrd mo,
- bool require_atomic_access = false);
+ bool require_atomic_access = false,
+ bool mismatched = false
+ );
// Store a card mark ordered after store_oop
Node* storeCM(Node* ctl,
diff --git a/src/share/vm/opto/library_call.cpp b/src/share/vm/opto/library_call.cpp
--- a/src/share/vm/opto/library_call.cpp
+++ b/src/share/vm/opto/library_call.cpp
@@ -2515,7 +2515,7 @@
// Update IdealKit memory.
__ sync_kit(this);
} __ else_(); {
- __ store(__ ctrl(), adr, val, type, alias_type->index(), mo, is_volatile);
+ __ store(__ ctrl(), adr, val, type, alias_type->index(), mo, is_volatile, mismatched);
} __ end_if();
// Final sync IdealKit and GraphKit.
final_sync(ideal);
> On Oct 14, 2015, at 10:48 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
> Good.
>
> Thanks,
> Vladimir
>
> On 10/12/15 4:46 PM, Roland Westrelin wrote:
>> Here is a new webrev that covers mismatched field accesses as well:
>>
>> http://cr.openjdk.java.net/~roland/8136473/webrev.03/
>>
>> Roland.
>>
>>> On Oct 5, 2015, at 1:22 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>
>>> So you are covering oops too. Okay then.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 10/5/15 5:12 PM, Roland Westrelin wrote:
>>>> Thanks for looking at this, Vladimir.
>>>>
>>>>> Mismatching access can be only for basic type as I understand. The code for mismatched = true could be rearranged by checking type != T_OBJECT first.
>>>>
>>>> People do such crazy things with Unsafe, I thought it was best to be prepared for anything.
>>>>
>>>>> Why you set mismatched only for array element? What about fields?
>>>>
>>>> You’re right, fields need to be handled the same way.
>>>>
>>>>> And next should be && I think (if you keep your code)
>>>>> if (type != T_OBJECT || !element->isa_narrowoop())
>>>>
>>>> Either we reach that test with a basic type that is not an object (type != T_OBJECT) and there’s a mismatch but
>>>> if we have element->array_element_basic_type() = T_NARROWOOP and type = T_OBJECT, then there’s no mismatch.
>>>>
>>>> Roland.
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 9/28/15 11:44 PM, Roland Westrelin wrote:
>>>>>> That fix wasn’t sufficient so here is a new webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~roland/8136473/webrev.02/
>>>>>>
>>>>>> On platforms that don’t support unaligned accesses we still have mismatched accesses because the java code of Unsafe.putIntUnaligned() is implemented in terms of putShort() and putByte() so my test case fails on sparc. I added a new boolean to MemNode to record whether the node is “mismatched” in addition to the “unaligned” boolean.
>>>>>>
>>>>>> Roland.
>>>>>>
>>>>>>> On Sep 22, 2015, at 2:50 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>>>>
>>>>>>> Okay.
>>>>>>>
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 9/22/15 8:47 PM, Roland Westrelin wrote:
>>>>>>>> For the record, it seems _unaligned is a reserved keyword for MSVC so I will change the field and function names to unaligned_access:
>>>>>>>>
>>>>>>>> diff --git a/src/share/vm/opto/memnode.hpp b/src/share/vm/opto/memnode.hpp
>>>>>>>> --- a/src/share/vm/opto/memnode.hpp
>>>>>>>> +++ b/src/share/vm/opto/memnode.hpp
>>>>>>>> @@ -40,7 +40,7 @@
>>>>>>>> // Load or Store, possibly throwing a NULL pointer exception
>>>>>>>> class MemNode : public Node {
>>>>>>>> private:
>>>>>>>> - bool _unaligned;
>>>>>>>> + bool _unaligned_access;
>>>>>>>> protected:
>>>>>>>> #ifdef ASSERT
>>>>>>>> const TypePtr* _adr_type; // What kind of memory is being addressed?
>>>>>>>>
>>>>>>>> @@ -129,8 +129,8 @@
>>>>>>>> // the given memory state? (The state may or may not be in(Memory).)
>>>>>>>> Node* can_see_stored_value(Node* st, PhaseTransform* phase) const;
>>>>>>>>
>>>>>>>> - void set_unaligned() { _unaligned = true; }
>>>>>>>> - bool is_unaligned() const { return _unaligned; }
>>>>>>>> + void set_unaligned_access() { _unaligned_access = true; }
>>>>>>>> + bool is_unaligned_access() const { return _unaligned_access; }
>>>>>>>>
>>>>>>>> #ifndef PRODUCT
>>>>>>>> static void dump_adr_type(const Node* mem, const TypePtr* adr_type, outputStream *st);
>>>>>>>>
>>>>>>>> Roland.
>>>>>>>>
>>>>>>>>> On Sep 22, 2015, at 9:43 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>>>>>>>>>
>>>>>>>>>> Roland I had a look too, the code looks good.
>>>>>>>>>
>>>>>>>>> Thanks, Michael.
>>>>>>>>>
>>>>>>>>> Roland.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -Michael
>>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Roland Westrelin
>>>>>>>>>> Sent: Monday, September 21, 2015 2:03 AM
>>>>>>>>>> To: hotspot compiler
>>>>>>>>>> Subject: Re: RFR(S): 8136473: failed: no mismatched stores, except on raw memory: StoreB StoreI
>>>>>>>>>>
>>>>>>>>>> Thanks for the review, Vladimir.
>>>>>>>>>>
>>>>>>>>>> Roland.
>>>>>>>>>>
>>>>>>>>>>> On Sep 21, 2015, at 3:32 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> This is good. Thank you for extending the test.
>>>>>>>>>>>
>>>>>>>>>>> Vladimir
>>>>>>>>>>>
>>>>>>>>>>> On 9/18/15 11:03 PM, Roland Westrelin wrote:
>>>>>>>>>>>>>>> Hmm, so you just relaxed the assert. You may need to fix EA too because it also checks for matching types(memory sizes).
>>>>>>>>>>>>>>> I remember we had problem with RAW accesses back when jsr292 was implemented. So EA gave up on RAW access and marks allocations as escaping. Also may be superword/vectorization will be affected.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In general it is very bad for C2 to have different memory on the same non-RAW memory.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> It already happens with vectorization. Do you see another solution (rather than relaxing the assert)? Putting a MemBarCPUOrder before and after the unaligned store?
>>>>>>>>>>>>>
>>>>>>>>>>>>> MemBarCPUOrder are definitely will help and it is simplest solution but, I think, it is too drastic.
>>>>>>>>>>>>> It affect control flow and other memory slices.
>>>>>>>>>>>>> We can try to mark such memory accesses or/and mark objects which have such accesses (if it is possible) and prevent vectorization and scalarization for them only. We should be able execute optimization on other memory slices then.
>>>>>>>>>>>>
>>>>>>>>>>>> Here is a new change:
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~roland/8136473/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> Unaligned accesses are explicitly marked as such.
>>>>>>>>>>>>
>>>>>>>>>>>> I added a test case with a non escaping allocation and it works fine. I also added test case with vectorization and if the accesses are effectively unaligned vectorization doesn't happen. One of the test cases causes an assertion to fire when the allocation is expanded: the logic in InitializeNode::complete_stores() is confused by the unaligned store so I made sure unaligned stores are not captured by the initialization.
>>>>>>>>>>>>
>>>>>>>>>>>> Roland.
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
More information about the hotspot-compiler-dev
mailing list