Skip to content

8390590: [BACKOUT] C2: Fix the memory around some intrinsics nodes #32429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vnkozlov wants to merge 1 commit into from
Closed

8390590: [BACKOUT] C2: Fix the memory around some intrinsics nodes #32429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 29 additions & 62 deletions src/hotspot/share/opto/graphKit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,14 @@
#include "opto/intrinsicnode.hpp"
#include "opto/locknode.hpp"
#include "opto/machnode.hpp"
#include "opto/memnode.hpp"
#include "opto/multnode.hpp"
#include "opto/narrowptrnode.hpp"
#include "opto/opaquenode.hpp"
#include "opto/opcodes.hpp"
#include "opto/parse.hpp"
#include "opto/reachability.hpp"
#include "opto/rootnode.hpp"
#include "opto/runtime.hpp"
#include "opto/subtypenode.hpp"
#include "opto/type.hpp"
#include "runtime/arguments.hpp"
#include "runtime/deoptimization.hpp"
#include "runtime/sharedRuntime.hpp"
Expand Down Expand Up @@ -4862,81 +4859,51 @@ void GraphKit::store_String_coder(Node* str, Node* value) {
value, TypeInt::BYTE, T_BYTE, IN_HEAP | MO_UNORDERED);
}

// If input and output memory types differ, capture the whole memory to preserve
// the dependency between preceding and subsequent loads/stores.
// For example, the following program:
// StoreB
// compress_string
// LoadB
// has this memory graph (use->def):
// LoadB -> compress_string -> CharMem
// ... -> StoreB -> ByteMem
// The intrinsic hides the dependency between LoadB and StoreB, causing
// the load to read from memory not containing the result of the StoreB.
// The correct memory graph should look like this:
// LoadB -> compress_string -> MergeMem -> StoreB
Node* GraphKit::capture_memory(const TypePtr*& combined_type, const TypePtr* src_type, const TypePtr* dst_type) {
// Capture src and dst memory state with a MergeMemNode
Node* GraphKit::capture_memory(const TypePtr* src_type, const TypePtr* dst_type) {
if (src_type == dst_type) {
// Types are equal, we don't need a MergeMemNode
combined_type = src_type;
return memory(src_type);
}
Node* mem = reset_memory();
set_all_memory(mem);
combined_type = TypePtr::BOTTOM;
return mem;
}

// If dst_type and src_type are different, str may have an anti-dependency with another node
// consuming src_type.
// For example:
// compress_string
// StoreC
// has this memory graph (use->def):
// compress_string -> MergeMem -> CharMem
// StoreC
// The scheduler needs to ensure that compress_string is not executed after StoreC, or it will read
// the wrong memory. For normal loads, the scheduler computes its anti-dependencies to ensure the
// memory it reads from is not killed. Since we do not compute anti-dependencies for
// StrCompressedCopyNode, manually insert a MemBar so the anti-dependency becomes use-def
// dependency:
// StoreC -> MemBar -> MergeMem -> compress_string -> MergeMem -> CharMem
// -------------------------------->
void GraphKit::memory_effect(Node* res_mem, const TypePtr* src_type, const TypePtr* dst_type) {
set_memory(res_mem, dst_type);
if (src_type != dst_type) {
Node* all_mem = reset_memory();
set_all_memory(all_mem);
Node* membar = new MemBarCPUOrderNode(C, C->get_alias_index(src_type), nullptr);
membar->init_req(TypeFunc::Control, control());
membar->init_req(TypeFunc::Memory, all_mem);
membar = _gvn.transform(membar);
set_control(_gvn.transform(new ProjNode(membar, TypeFunc::Control)));
set_memory(_gvn.transform(new ProjNode(membar, TypeFunc::Memory)), src_type);
}
MergeMemNode* merge = MergeMemNode::make(map()->memory());
record_for_igvn(merge); // fold it up later, if possible
int src_idx = C->get_alias_index(src_type);
int dst_idx = C->get_alias_index(dst_type);
merge->set_memory_at(src_idx, memory(src_idx));
merge->set_memory_at(dst_idx, memory(dst_idx));
return merge;
}

Node* GraphKit::compress_string(Node* src, const TypeAryPtr* src_type, Node* dst, Node* count) {
assert(Matcher::match_rule_supported(Op_StrCompressedCopy), "Intrinsic not supported");
assert(src_type == TypeAryPtr::BYTES || src_type == TypeAryPtr::CHARS, "invalid source type");
const TypePtr* dst_type = TypeAryPtr::BYTES;
const TypePtr* adr_type;
Node* mem = capture_memory(adr_type, src_type, dst_type);
StrCompressedCopyNode* str = new StrCompressedCopyNode(control(), mem, adr_type, src, dst, count);
// If input and output memory types differ, capture both states to preserve
// the dependency between preceding and subsequent loads/stores.
// For example, the following program:
// StoreB
// compress_string
// LoadB
// has this memory graph (use->def):
// LoadB -> compress_string -> CharMem
// ... -> StoreB -> ByteMem
// The intrinsic hides the dependency between LoadB and StoreB, causing
// the load to read from memory not containing the result of the StoreB.
// The correct memory graph should look like this:
// LoadB -> compress_string -> MergeMem(CharMem, StoreB(ByteMem))
Node* mem = capture_memory(src_type, TypeAryPtr::BYTES);
StrCompressedCopyNode* str = new StrCompressedCopyNode(control(), mem, src, dst, count);
Node* res_mem = _gvn.transform(new SCMemProjNode(_gvn.transform(str)));
memory_effect(res_mem, src_type, dst_type);
set_memory(res_mem, TypeAryPtr::BYTES);
return str;
}

void GraphKit::inflate_string(Node* src, Node* dst, const TypeAryPtr* dst_type, Node* count) {
assert(Matcher::match_rule_supported(Op_StrInflatedCopy), "Intrinsic not supported");
assert(dst_type == TypeAryPtr::BYTES || dst_type == TypeAryPtr::CHARS, "invalid dest type");
const TypePtr* src_type = TypeAryPtr::BYTES;
const TypePtr* adr_type;
Node* mem = capture_memory(adr_type, src_type, dst_type);
StrInflatedCopyNode* str = new StrInflatedCopyNode(control(), mem, adr_type, src, dst, count);
Node* res_mem = _gvn.transform(str);
memory_effect(res_mem, src_type, dst_type);
// Capture src and dst memory (see comment in 'compress_string').
Node* mem = capture_memory(TypeAryPtr::BYTES, dst_type);
StrInflatedCopyNode* str = new StrInflatedCopyNode(control(), mem, src, dst, count);
set_memory(_gvn.transform(str), dst_type);
}

void GraphKit::inflate_string_slow(Node* src, Node* dst, Node* start, Node* count) {
Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/share/opto/graphKit.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,8 +886,7 @@ class GraphKit : public Phase {
Node* load_String_coder(Node* str, bool set_ctrl);
void store_String_value(Node* str, Node* value);
void store_String_coder(Node* str, Node* value);
Node* capture_memory(const TypePtr*& combined_type, const TypePtr* src_type, const TypePtr* dst_type);
void memory_effect(Node* res_mem, const TypePtr* src_type, const TypePtr* dst_type);
Node* capture_memory(const TypePtr* src_type, const TypePtr* dst_type);
Node* compress_string(Node* src, const TypeAryPtr* src_type, Node* dst, Node* count);
void inflate_string(Node* src, Node* dst, const TypeAryPtr* dst_type, Node* count);
void inflate_string_slow(Node* src, Node* dst, Node* start, Node* count);
Expand Down
2 changes: 2 additions & 0 deletions src/hotspot/share/opto/intrinsicnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ const Type* StrIntrinsicNode::Value(PhaseGVN* phase) const {
return bottom_type();
}

uint StrIntrinsicNode::size_of() const { return sizeof(*this); }

//=============================================================================
//------------------------------Ideal------------------------------------------
// Return a node which is more "ideal" than the current node. Strip out
Expand Down
Loading