Optimizing: Clean up after const-string sharpening.
Do not look up the String for JIT, just check if it's in the
dex cache. Strings on hot paths should already be resolved
and we don't want to unnecessarily increase JIT compile time
to have a chance of improving a cold path.
Also, change the enum LinkerPatchType to be an inner enum
class of LinkerPatch and clean up casts between pointers and
uint64_t.
Change-Id: Ia6e0513af1a84ce94a3b30edac0c592157d374ec
diff --git a/compiler/compiled_method.h b/compiler/compiled_method.h
index acd5089..b5c99e8 100644
--- a/compiler/compiled_method.h
+++ b/compiler/compiled_method.h
@@ -159,29 +159,27 @@
using DefaultSrcMap = SrcMap<std::allocator<SrcMapElem>>;
-
-enum LinkerPatchType {
- kLinkerPatchRecordPosition, // Just record patch position for patchoat.
- kLinkerPatchMethod,
- kLinkerPatchCall,
- kLinkerPatchCallRelative, // NOTE: Actual patching is instruction_set-dependent.
- kLinkerPatchType,
- kLinkerPatchString,
- kLinkerPatchStringRelative, // NOTE: Actual patching is instruction_set-dependent.
- kLinkerPatchDexCacheArray, // NOTE: Actual patching is instruction_set-dependent.
-};
-std::ostream& operator<<(std::ostream& os, const LinkerPatchType& type);
-
class LinkerPatch {
public:
+ enum class Type {
+ kRecordPosition, // Just record patch position for patchoat.
+ kMethod,
+ kCall,
+ kCallRelative, // NOTE: Actual patching is instruction_set-dependent.
+ kType,
+ kString,
+ kStringRelative, // NOTE: Actual patching is instruction_set-dependent.
+ kDexCacheArray, // NOTE: Actual patching is instruction_set-dependent.
+ };
+
static LinkerPatch RecordPosition(size_t literal_offset) {
- return LinkerPatch(literal_offset, kLinkerPatchRecordPosition, /* target_dex_file */ nullptr);
+ return LinkerPatch(literal_offset, Type::kRecordPosition, /* target_dex_file */ nullptr);
}
static LinkerPatch MethodPatch(size_t literal_offset,
const DexFile* target_dex_file,
uint32_t target_method_idx) {
- LinkerPatch patch(literal_offset, kLinkerPatchMethod, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kMethod, target_dex_file);
patch.method_idx_ = target_method_idx;
return patch;
}
@@ -189,7 +187,7 @@
static LinkerPatch CodePatch(size_t literal_offset,
const DexFile* target_dex_file,
uint32_t target_method_idx) {
- LinkerPatch patch(literal_offset, kLinkerPatchCall, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kCall, target_dex_file);
patch.method_idx_ = target_method_idx;
return patch;
}
@@ -197,7 +195,7 @@
static LinkerPatch RelativeCodePatch(size_t literal_offset,
const DexFile* target_dex_file,
uint32_t target_method_idx) {
- LinkerPatch patch(literal_offset, kLinkerPatchCallRelative, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kCallRelative, target_dex_file);
patch.method_idx_ = target_method_idx;
return patch;
}
@@ -205,7 +203,7 @@
static LinkerPatch TypePatch(size_t literal_offset,
const DexFile* target_dex_file,
uint32_t target_type_idx) {
- LinkerPatch patch(literal_offset, kLinkerPatchType, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kType, target_dex_file);
patch.type_idx_ = target_type_idx;
return patch;
}
@@ -213,7 +211,7 @@
static LinkerPatch StringPatch(size_t literal_offset,
const DexFile* target_dex_file,
uint32_t target_string_idx) {
- LinkerPatch patch(literal_offset, kLinkerPatchString, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kString, target_dex_file);
patch.string_idx_ = target_string_idx;
return patch;
}
@@ -222,7 +220,7 @@
const DexFile* target_dex_file,
uint32_t pc_insn_offset,
uint32_t target_string_idx) {
- LinkerPatch patch(literal_offset, kLinkerPatchStringRelative, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kStringRelative, target_dex_file);
patch.string_idx_ = target_string_idx;
patch.pc_insn_offset_ = pc_insn_offset;
return patch;
@@ -233,7 +231,7 @@
uint32_t pc_insn_offset,
size_t element_offset) {
DCHECK(IsUint<32>(element_offset));
- LinkerPatch patch(literal_offset, kLinkerPatchDexCacheArray, target_dex_file);
+ LinkerPatch patch(literal_offset, Type::kDexCacheArray, target_dex_file);
patch.pc_insn_offset_ = pc_insn_offset;
patch.element_offset_ = element_offset;
return patch;
@@ -246,15 +244,15 @@
return literal_offset_;
}
- LinkerPatchType Type() const {
+ Type GetType() const {
return patch_type_;
}
bool IsPcRelative() const {
- switch (Type()) {
- case kLinkerPatchCallRelative:
- case kLinkerPatchStringRelative:
- case kLinkerPatchDexCacheArray:
+ switch (GetType()) {
+ case Type::kCallRelative:
+ case Type::kStringRelative:
+ case Type::kDexCacheArray:
return true;
default:
return false;
@@ -262,48 +260,49 @@
}
MethodReference TargetMethod() const {
- DCHECK(patch_type_ == kLinkerPatchMethod ||
- patch_type_ == kLinkerPatchCall || patch_type_ == kLinkerPatchCallRelative);
+ DCHECK(patch_type_ == Type::kMethod ||
+ patch_type_ == Type::kCall ||
+ patch_type_ == Type::kCallRelative);
return MethodReference(target_dex_file_, method_idx_);
}
const DexFile* TargetTypeDexFile() const {
- DCHECK(patch_type_ == kLinkerPatchType);
+ DCHECK(patch_type_ == Type::kType);
return target_dex_file_;
}
uint32_t TargetTypeIndex() const {
- DCHECK(patch_type_ == kLinkerPatchType);
+ DCHECK(patch_type_ == Type::kType);
return type_idx_;
}
const DexFile* TargetStringDexFile() const {
- DCHECK(patch_type_ == kLinkerPatchString || patch_type_ == kLinkerPatchStringRelative);
+ DCHECK(patch_type_ == Type::kString || patch_type_ == Type::kStringRelative);
return target_dex_file_;
}
uint32_t TargetStringIndex() const {
- DCHECK(patch_type_ == kLinkerPatchString || patch_type_ == kLinkerPatchStringRelative);
+ DCHECK(patch_type_ == Type::kString || patch_type_ == Type::kStringRelative);
return string_idx_;
}
const DexFile* TargetDexCacheDexFile() const {
- DCHECK(patch_type_ == kLinkerPatchDexCacheArray);
+ DCHECK(patch_type_ == Type::kDexCacheArray);
return target_dex_file_;
}
size_t TargetDexCacheElementOffset() const {
- DCHECK(patch_type_ == kLinkerPatchDexCacheArray);
+ DCHECK(patch_type_ == Type::kDexCacheArray);
return element_offset_;
}
uint32_t PcInsnOffset() const {
- DCHECK(patch_type_ == kLinkerPatchStringRelative || patch_type_ == kLinkerPatchDexCacheArray);
+ DCHECK(patch_type_ == Type::kStringRelative || patch_type_ == Type::kDexCacheArray);
return pc_insn_offset_;
}
private:
- LinkerPatch(size_t literal_offset, LinkerPatchType patch_type, const DexFile* target_dex_file)
+ LinkerPatch(size_t literal_offset, Type patch_type, const DexFile* target_dex_file)
: target_dex_file_(target_dex_file),
literal_offset_(literal_offset),
patch_type_(patch_type) {
@@ -316,7 +315,7 @@
const DexFile* target_dex_file_;
uint32_t literal_offset_ : 24; // Method code size up to 16MiB.
- LinkerPatchType patch_type_ : 8;
+ Type patch_type_ : 8;
union {
uint32_t cmp1_; // Used for relational operators.
uint32_t method_idx_; // Method index for Call/Method patches.
@@ -341,6 +340,7 @@
friend bool operator==(const LinkerPatch& lhs, const LinkerPatch& rhs);
friend bool operator<(const LinkerPatch& lhs, const LinkerPatch& rhs);
};
+std::ostream& operator<<(std::ostream& os, const LinkerPatch::Type& type);
inline bool operator==(const LinkerPatch& lhs, const LinkerPatch& rhs) {
return lhs.literal_offset_ == rhs.literal_offset_ &&
diff --git a/compiler/linker/arm/relative_patcher_arm_base.cc b/compiler/linker/arm/relative_patcher_arm_base.cc
index 682b008..d4dd978 100644
--- a/compiler/linker/arm/relative_patcher_arm_base.cc
+++ b/compiler/linker/arm/relative_patcher_arm_base.cc
@@ -112,7 +112,7 @@
}
}
for (const LinkerPatch& patch : compiled_method->GetPatches()) {
- if (patch.Type() == kLinkerPatchCallRelative) {
+ if (patch.GetType() == LinkerPatch::Type::kCallRelative) {
unprocessed_patches_.emplace_back(patch.TargetMethod(),
quick_code_offset + patch.LiteralOffset());
}
diff --git a/compiler/linker/arm64/relative_patcher_arm64.cc b/compiler/linker/arm64/relative_patcher_arm64.cc
index 0549327..e3e3121 100644
--- a/compiler/linker/arm64/relative_patcher_arm64.cc
+++ b/compiler/linker/arm64/relative_patcher_arm64.cc
@@ -31,8 +31,9 @@
namespace {
inline bool IsAdrpPatch(const LinkerPatch& patch) {
- LinkerPatchType type = patch.Type();
- return (type == kLinkerPatchStringRelative || type == kLinkerPatchDexCacheArray) &&
+ LinkerPatch::Type type = patch.GetType();
+ return
+ (type == LinkerPatch::Type::kStringRelative || type == LinkerPatch::Type::kDexCacheArray) &&
patch.LiteralOffset() == patch.PcInsnOffset();
}
@@ -209,11 +210,11 @@
} else {
if ((insn & 0xfffffc00) == 0x91000000) {
// ADD immediate, 64-bit with imm12 == 0 (unset).
- DCHECK(patch.Type() == kLinkerPatchStringRelative) << patch.Type();
+ DCHECK(patch.GetType() == LinkerPatch::Type::kStringRelative) << patch.GetType();
shift = 0u; // No shift for ADD.
} else {
// LDR 32-bit or 64-bit with imm12 == 0 (unset).
- DCHECK(patch.Type() == kLinkerPatchDexCacheArray) << patch.Type();
+ DCHECK(patch.GetType() == LinkerPatch::Type::kDexCacheArray) << patch.GetType();
DCHECK_EQ(insn & 0xbffffc00, 0xb9400000) << std::hex << insn;
}
if (kIsDebugBuild) {
@@ -292,9 +293,10 @@
return false;
}
- // And since kLinkerPatchStringRelative is using the result of the ADRP for an ADD immediate,
- // check for that as well. We generalize a bit to include ADD/ADDS/SUB/SUBS immediate that
- // either uses the ADRP destination or stores the result to a different register.
+ // And since LinkerPatch::Type::kStringRelative is using the result of the ADRP
+ // for an ADD immediate, check for that as well. We generalize a bit to include
+ // ADD/ADDS/SUB/SUBS immediate that either uses the ADRP destination or stores
+ // the result to a different register.
if ((next_insn & 0x1f000000) == 0x11000000 &&
((((next_insn >> 5) ^ adrp) & 0x1f) == 0 || ((next_insn ^ adrp) & 0x1f) != 0)) {
return false;
diff --git a/compiler/linker/relative_patcher_test.h b/compiler/linker/relative_patcher_test.h
index c9fb543..bf61ea0 100644
--- a/compiler/linker/relative_patcher_test.h
+++ b/compiler/linker/relative_patcher_test.h
@@ -142,26 +142,26 @@
patched_code_.assign(code.begin(), code.end());
code = ArrayRef<const uint8_t>(patched_code_);
for (const LinkerPatch& patch : compiled_method->GetPatches()) {
- if (patch.Type() == kLinkerPatchCallRelative) {
+ if (patch.GetType() == LinkerPatch::Type::kCallRelative) {
auto result = method_offset_map_.FindMethodOffset(patch.TargetMethod());
uint32_t target_offset =
result.first ? result.second : kTrampolineOffset + compiled_method->CodeDelta();
patcher_->PatchCall(&patched_code_, patch.LiteralOffset(),
offset + patch.LiteralOffset(), target_offset);
- } else if (patch.Type() == kLinkerPatchDexCacheArray) {
+ } else if (patch.GetType() == LinkerPatch::Type::kDexCacheArray) {
uint32_t target_offset = dex_cache_arrays_begin_ + patch.TargetDexCacheElementOffset();
patcher_->PatchPcRelativeReference(&patched_code_,
patch,
offset + patch.LiteralOffset(),
target_offset);
- } else if (patch.Type() == kLinkerPatchStringRelative) {
+ } else if (patch.GetType() == LinkerPatch::Type::kStringRelative) {
uint32_t target_offset = string_index_to_offset_map_.Get(patch.TargetStringIndex());
patcher_->PatchPcRelativeReference(&patched_code_,
patch,
offset + patch.LiteralOffset(),
target_offset);
} else {
- LOG(FATAL) << "Bad patch type. " << patch.Type();
+ LOG(FATAL) << "Bad patch type. " << patch.GetType();
UNREACHABLE();
}
}
diff --git a/compiler/oat_writer.cc b/compiler/oat_writer.cc
index a7d574c..3a67b1e 100644
--- a/compiler/oat_writer.cc
+++ b/compiler/oat_writer.cc
@@ -1116,8 +1116,8 @@
quick_code = ArrayRef<const uint8_t>(patched_code_);
for (const LinkerPatch& patch : compiled_method->GetPatches()) {
uint32_t literal_offset = patch.LiteralOffset();
- switch (patch.Type()) {
- case kLinkerPatchCallRelative: {
+ switch (patch.GetType()) {
+ case LinkerPatch::Type::kCallRelative: {
// NOTE: Relative calls across oat files are not supported.
uint32_t target_offset = GetTargetOffset(patch);
writer_->relative_patcher_->PatchCall(&patched_code_,
@@ -1126,7 +1126,7 @@
target_offset);
break;
}
- case kLinkerPatchDexCacheArray: {
+ case LinkerPatch::Type::kDexCacheArray: {
uint32_t target_offset = GetDexCacheOffset(patch);
writer_->relative_patcher_->PatchPcRelativeReference(&patched_code_,
patch,
@@ -1134,7 +1134,7 @@
target_offset);
break;
}
- case kLinkerPatchStringRelative: {
+ case LinkerPatch::Type::kStringRelative: {
uint32_t target_offset = GetTargetObjectOffset(GetTargetString(patch));
writer_->relative_patcher_->PatchPcRelativeReference(&patched_code_,
patch,
@@ -1142,28 +1142,28 @@
target_offset);
break;
}
- case kLinkerPatchCall: {
+ case LinkerPatch::Type::kCall: {
uint32_t target_offset = GetTargetOffset(patch);
PatchCodeAddress(&patched_code_, literal_offset, target_offset);
break;
}
- case kLinkerPatchMethod: {
+ case LinkerPatch::Type::kMethod: {
ArtMethod* method = GetTargetMethod(patch);
PatchMethodAddress(&patched_code_, literal_offset, method);
break;
}
- case kLinkerPatchString: {
+ case LinkerPatch::Type::kString: {
mirror::String* string = GetTargetString(patch);
PatchObjectAddress(&patched_code_, literal_offset, string);
break;
}
- case kLinkerPatchType: {
+ case LinkerPatch::Type::kType: {
mirror::Class* type = GetTargetType(patch);
PatchObjectAddress(&patched_code_, literal_offset, type);
break;
}
default: {
- DCHECK_EQ(patch.Type(), kLinkerPatchRecordPosition);
+ DCHECK_EQ(patch.GetType(), LinkerPatch::Type::kRecordPosition);
break;
}
}
diff --git a/compiler/optimizing/sharpening.cc b/compiler/optimizing/sharpening.cc
index 45ae336..7a1bb31 100644
--- a/compiler/optimizing/sharpening.cc
+++ b/compiler/optimizing/sharpening.cc
@@ -16,6 +16,7 @@
#include "sharpening.h"
+#include "base/casts.h"
#include "class_linker.h"
#include "code_generator.h"
#include "driver/dex_compilation_unit.h"
@@ -180,46 +181,42 @@
// MIPS/MIPS64 or compiler_driver_test. Do not sharpen.
desired_load_kind = HLoadString::LoadKind::kDexCacheViaMethod;
} else {
- DCHECK(ContainsElement(compiler_driver_->GetDexFilesForOatFile(),
- &load_string->GetDexFile()));
+ DCHECK(ContainsElement(compiler_driver_->GetDexFilesForOatFile(), &dex_file));
is_in_dex_cache = true;
desired_load_kind = codegen_->GetCompilerOptions().GetCompilePic()
? HLoadString::LoadKind::kBootImageLinkTimePcRelative
: HLoadString::LoadKind::kBootImageLinkTimeAddress;
}
+ } else if (runtime->UseJit()) {
+ // TODO: Make sure we don't set the "compile PIC" flag for JIT as that's bogus.
+ // DCHECK(!codegen_->GetCompilerOptions().GetCompilePic());
+ mirror::String* string = dex_cache->GetResolvedString(string_index);
+ is_in_dex_cache = (string != nullptr);
+ if (string != nullptr && runtime->GetHeap()->ObjectIsInBootImageSpace(string)) {
+ desired_load_kind = HLoadString::LoadKind::kBootImageAddress;
+ address = reinterpret_cast64<uint64_t>(string);
+ } else {
+ // Note: If the string is not in the dex cache, the instruction needs environment
+ // and will not be inlined across dex files. Within a dex file, the slow-path helper
+ // loads the correct string and inlined frames are used correctly for OOM stack trace.
+ // TODO: Write a test for this.
+ desired_load_kind = HLoadString::LoadKind::kDexCacheAddress;
+ void* dex_cache_element_address = &dex_cache->GetStrings()[string_index];
+ address = reinterpret_cast64<uint64_t>(dex_cache_element_address);
+ }
} else {
- // Not compiling boot image. Try to lookup the string without allocating if not found.
+ // AOT app compilation. Try to lookup the string without allocating if not found.
mirror::String* string = class_linker->LookupString(dex_file, string_index, dex_cache);
- if (runtime->UseJit()) {
- // TODO: Make sure we don't set the "compile PIC" flag for JIT as that's bogus.
- // DCHECK(!codegen_->GetCompilerOptions().GetCompilePic());
- is_in_dex_cache = (string != nullptr);
- if (string != nullptr && runtime->GetHeap()->ObjectIsInBootImageSpace(string)) {
- desired_load_kind = HLoadString::LoadKind::kBootImageAddress;
- // Convert to uintptr_t first to avoid sign-extension if a 32-bit pointer is "signed."
- address = static_cast<uint64_t>(reinterpret_cast<uintptr_t>(string));
- } else {
- // Note: If the string is not in the dex cache, the instruction needs environment
- // and will not be inlined across dex files. Within a dex file, the slow-path helper
- // loads the correct string and inlined frames are used correctly for OOM stack trace.
- // TODO: Write a test for this.
- desired_load_kind = HLoadString::LoadKind::kDexCacheAddress;
- void* dex_cache_element_address = &dex_cache->GetStrings()[string_index];
- // Convert to uintptr_t first to avoid sign-extension if a 32-bit pointer is "signed."
- address = static_cast<uint64_t>(reinterpret_cast<uintptr_t>(dex_cache_element_address));
- }
- } else if (string != nullptr && runtime->GetHeap()->ObjectIsInBootImageSpace(string)) {
+ if (string != nullptr && runtime->GetHeap()->ObjectIsInBootImageSpace(string)) {
if (codegen_->GetCompilerOptions().GetCompilePic()) {
// Use PC-relative load from the dex cache if the dex file belongs
// to the oat file that we're currently compiling.
- desired_load_kind =
- ContainsElement(compiler_driver_->GetDexFilesForOatFile(), &load_string->GetDexFile())
- ? HLoadString::LoadKind::kDexCachePcRelative
- : HLoadString::LoadKind::kDexCacheViaMethod;
+ desired_load_kind = ContainsElement(compiler_driver_->GetDexFilesForOatFile(), &dex_file)
+ ? HLoadString::LoadKind::kDexCachePcRelative
+ : HLoadString::LoadKind::kDexCacheViaMethod;
} else {
desired_load_kind = HLoadString::LoadKind::kBootImageAddress;
- // Convert to uintptr_t first to avoid sign-extension if a 32-bit pointer is "signed."
- address = static_cast<uint64_t>(reinterpret_cast<uintptr_t>(string));
+ address = reinterpret_cast64<uint64_t>(string);
}
} else {
// Not JIT and the string is not in boot image.