Shark checkcast and instanceof improvements (part 3)
Gary Benson
gbenson at redhat.com
Fri Jun 5 11:15:35 PDT 2009
Hi all,
This commit adds special handling to instanceof instructions that
are immediately followed by an ifeq or an ifne. The instructions
are compiled as a pair, with the value checked by the instanceof
annotated with updated type information in the branch of the if
corresponding to the instanceof returning 1. This means that if
the branch then contains a checkcast then that checkcast will be
eliminated with a static check.
Cheers,
Gary
--
http://gbenson.net/
-------------- next part --------------
diff -r 8ba2783ba3b0 -r afa643fbffde ChangeLog
--- a/ChangeLog Fri Jun 05 10:25:13 2009 -0400
+++ b/ChangeLog Fri Jun 05 19:11:26 2009 +0100
@@ -1,4 +1,33 @@
-2009-06-05 Omair Majid <omajid at redhat.com>
+2009-06-06 Gary Benson <gbenson at redhat.com>
+
+ * ports/hotspot/src/share/vm/shark/sharkBlock.hpp
+ (SharkBlock::maybe_do_instanceof_if): New method.
+ * ports/hotspot/src/share/vm/shark/sharkBlock.cpp
+ (SharkBlock::maybe_do_instanceof_if): New method.
+ (SharkBlock::parse_bytecode): Use the above for
+ instanceof immediately followed by ifeq or ifne.
+
+ * ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.hpp
+ (SharkTopLevelBlock::do_if_helper): New method.
+ (SharkTopLevelBlock::static_subtype_check): Likewise.
+ (SharkTopLevelBlock::maybe_do_instanceof_if): Likewise.
+ (SharkTopLevelBlock::do_optimized_instance_check): Removed.
+ * ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.cpp
+ (SharkTopLevelBlock::do_if_helper): New method.
+ (SharkTopLevelBlock::do_if): Use the above.
+ (SharkTopLevelBlock::static_subtype_check): New method.
+ (SharkTopLevelBlock::do_instance_check): Use the above.
+ (SharkTopLevelBlock::maybe_do_instanceof_if): New method.
+ (SharkTopLevelBlock::do_optimized_instance_check): Removed.
+ (SharkTopLevelBlock::do_full_instance_check): Removed
+ now-unnecessary state copy and merge.
+
+ * ports/hotspot/src/share/vm/shark/sharkState.hpp
+ (SharkState::replace_all): New method.
+ * ports/hotspot/src/share/vm/shark/sharkState.cpp
+ (SharkState::replace_all): Likewise.
+
+2009-06-06 Omair Majid <omajid at redhat.com>
* patches/icedtea-webstart.patch: Make javax.jnlp package visible to
javac.
diff -r 8ba2783ba3b0 -r afa643fbffde ports/hotspot/src/share/vm/shark/sharkBlock.cpp
--- a/ports/hotspot/src/share/vm/shark/sharkBlock.cpp Fri Jun 05 10:25:13 2009 -0400
+++ b/ports/hotspot/src/share/vm/shark/sharkBlock.cpp Fri Jun 05 19:11:26 2009 +0100
@@ -825,8 +825,38 @@
do_call();
break;
+ case Bytecodes::_instanceof:
+ // This is a very common construct:
+ //
+ // if (object instanceof Klass) {
+ // something = (Klass) object;
+ // ...
+ // }
+ //
+ // which gets compiled to something like this:
+ //
+ // 28: aload 9
+ // 30: instanceof <Class Klass>
+ // 33: ifeq 52
+ // 36: aload 9
+ // 38: checkcast <Class Klass>
+ //
+ // Handling both bytecodes at once allows us
+ // to eliminate the checkcast.
+ if (iter()->next_bci() < limit &&
+ (iter()->next_bc() == Bytecodes::_ifeq ||
+ iter()->next_bc() == Bytecodes::_ifne) &&
+ (!UseLoopSafepoints ||
+ iter()->next_get_dest() > iter()->next_bci())) {
+ if (maybe_do_instanceof_if()) {
+ iter()->next();
+ if (SharkTraceBytecodes)
+ tty->print_cr("%4d: %s", bci(), Bytecodes::name(bc()));
+ break;
+ }
+ }
+ // fall through
case Bytecodes::_checkcast:
- case Bytecodes::_instanceof:
do_instance_check();
break;
@@ -1225,6 +1255,11 @@
ShouldNotCallThis();
}
+bool SharkBlock::maybe_do_instanceof_if()
+{
+ ShouldNotCallThis();
+}
+
void SharkBlock::do_new()
{
ShouldNotCallThis();
diff -r 8ba2783ba3b0 -r afa643fbffde ports/hotspot/src/share/vm/shark/sharkBlock.hpp
--- a/ports/hotspot/src/share/vm/shark/sharkBlock.hpp Fri Jun 05 10:25:13 2009 -0400
+++ b/ports/hotspot/src/share/vm/shark/sharkBlock.hpp Fri Jun 05 19:11:26 2009 +0100
@@ -326,6 +326,7 @@
// checkcast and instanceof
protected:
virtual void do_instance_check();
+ virtual bool maybe_do_instanceof_if();
// new and *newarray
protected:
diff -r 8ba2783ba3b0 -r afa643fbffde ports/hotspot/src/share/vm/shark/sharkState.cpp
--- a/ports/hotspot/src/share/vm/shark/sharkState.cpp Fri Jun 05 10:25:13 2009 -0400
+++ b/ports/hotspot/src/share/vm/shark/sharkState.cpp Fri Jun 05 19:11:26 2009 +0100
@@ -225,6 +225,21 @@
set_has_safepointed(this->has_safepointed() && other->has_safepointed());
}
+void SharkState::replace_all(SharkValue* old_value, SharkValue* new_value)
+{
+ // Local variables
+ for (int i = 0; i < max_locals(); i++) {
+ if (local(i) == old_value)
+ set_local(i, new_value);
+ }
+
+ // Expression stack
+ for (int i = 0; i < stack_depth(); i++) {
+ if (stack(i) == old_value)
+ set_stack(i, new_value);
+ }
+}
+
void SharkState::decache_for_Java_call(ciMethod* callee)
{
assert(function() && method(), "you cannot decache here");
diff -r 8ba2783ba3b0 -r afa643fbffde ports/hotspot/src/share/vm/shark/sharkState.hpp
--- a/ports/hotspot/src/share/vm/shark/sharkState.hpp Fri Jun 05 10:25:13 2009 -0400
+++ b/ports/hotspot/src/share/vm/shark/sharkState.hpp Fri Jun 05 19:11:26 2009 +0100
@@ -187,6 +187,10 @@
llvm::BasicBlock* other_block,
llvm::BasicBlock* this_block);
+ // Value replacement
+ public:
+ void replace_all(SharkValue* old_value, SharkValue* new_value);
+
// Cache and decache
public:
void decache_for_Java_call(ciMethod* callee);
diff -r 8ba2783ba3b0 -r afa643fbffde ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.cpp
--- a/ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.cpp Fri Jun 05 10:25:13 2009 -0400
+++ b/ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.cpp Fri Jun 05 19:11:26 2009 +0100
@@ -772,8 +772,11 @@
}
// All propagation of state from one block to the next (via
-// dest->add_incoming) is handled by the next three methods
-// (do_branch, do_if and do_switch) and by handle_exception.
+// dest->add_incoming) is handled by these methods:
+// do_branch
+// do_if_helper
+// do_switch
+// handle_exception
void SharkTopLevelBlock::do_branch(int successor_index)
{
@@ -795,16 +798,24 @@
llvm_a = a->jint_value();
llvm_b = b->jint_value();
}
+ do_if_helper(p, llvm_b, llvm_a, current_state(), current_state());
+}
+void SharkTopLevelBlock::do_if_helper(ICmpInst::Predicate p,
+ Value* b,
+ Value* a,
+ SharkState* if_taken_state,
+ SharkState* not_taken_state)
+{
SharkTopLevelBlock *if_taken = successor(ciTypeFlow::IF_TAKEN);
SharkTopLevelBlock *not_taken = successor(ciTypeFlow::IF_NOT_TAKEN);
builder()->CreateCondBr(
- builder()->CreateICmp(p, llvm_a, llvm_b),
+ builder()->CreateICmp(p, a, b),
if_taken->entry_block(), not_taken->entry_block());
- if_taken->add_incoming(current_state());
- not_taken->add_incoming(current_state());
+ if_taken->add_incoming(if_taken_state);
+ not_taken->add_incoming(not_taken_state);
}
void SharkTopLevelBlock::do_switch()
@@ -1164,40 +1175,48 @@
current_state()->set_has_safepointed(true);
}
+bool SharkTopLevelBlock::static_subtype_check(ciKlass* check_klass,
+ ciKlass* object_klass)
+{
+ // If the class we're checking against is java.lang.Object
+ // then this is a no brainer. Apparently this can happen
+ // in reflective code...
+ if (check_klass == function()->env()->Object_klass())
+ return true;
+
+ // Perform a subtype check. NB in opto's code for this
+ // (GraphKit::static_subtype_check) it says that static
+ // interface types cannot be trusted, and if opto can't
+ // trust them then I assume we can't either.
+ if (!object_klass->is_interface()) {
+ if (object_klass == check_klass)
+ return true;
+
+ if (object_klass->is_loaded() && check_klass->is_loaded()) {
+ if (object_klass->is_subtype_of(check_klass))
+ return true;
+ }
+ }
+
+ return false;
+}
+
void SharkTopLevelBlock::do_instance_check()
{
// Get the class we're checking against
bool will_link;
ciKlass *check_klass = iter()->get_klass(will_link);
- // If the class we're checking against is java.lang.Object
- // then this is a no brainer. Apparently this can happen
- // in reflective code...
- if (check_klass == function()->env()->Object_klass()) {
- do_optimized_instance_check();
- return;
- }
-
// Get the class of the object we're checking
ciKlass *object_klass = xstack(0)->type()->as_klass();
- // If the classes are defined enough now then we
- // don't need a runtime check. NB opto's code for
- // this (GraphKit::static_subtype_check) says we
- // cannot trust static interface types yet, hence
- // the extra check
- if (!object_klass->is_interface()) {
- if (object_klass == check_klass) {
- do_optimized_instance_check();
- return;
+ // Can we optimize this check away?
+ if (static_subtype_check(check_klass, object_klass)) {
+ if (bc() == Bytecodes::_instanceof) {
+ pop();
+ push(SharkValue::jint_constant(1));
}
-
- if (object_klass->is_loaded() && check_klass->is_loaded()) {
- if (object_klass->is_subtype_of(check_klass)) {
- do_optimized_instance_check();
- return;
- }
- }
+ return;
}
// Need to check this one at runtime
@@ -1207,14 +1226,69 @@
do_trapping_instance_check(check_klass);
}
-void SharkTopLevelBlock::do_optimized_instance_check()
+bool SharkTopLevelBlock::maybe_do_instanceof_if()
{
- if (bc() == Bytecodes::_instanceof) {
- pop();
- push(SharkValue::jint_constant(1));
+ // Get the class we're checking against
+ bool will_link;
+ ciKlass *check_klass = iter()->get_klass(will_link);
+
+ // If the class is unloaded then the instanceof
+ // cannot possibly succeed.
+ if (!will_link)
+ return false;
+
+ // Keep a copy of the object we're checking
+ SharkValue *old_object = xstack(0);
+
+ // Get the class of the object we're checking
+ ciKlass *object_klass = old_object->type()->as_klass();
+
+ // If the instanceof can be optimized away at compile time
+ // then any subsequent checkcasts will be too so we handle
+ // it normally.
+ if (static_subtype_check(check_klass, object_klass))
+ return false;
+
+ // Perform the instance check
+ do_full_instance_check(check_klass);
+ Value *result = pop()->jint_value();
+
+ // Create the casted object
+ SharkValue *new_object = SharkValue::create_generic(
+ check_klass, old_object->jobject_value(), old_object->zero_checked());
+
+ // Create two copies of the current state, one with the
+ // original object and one with all instances of the
+ // original object replaced with the new, casted object.
+ SharkState *new_state = current_state();
+ SharkState *old_state = new_state->copy();
+ new_state->replace_all(old_object, new_object);
+
+ // Perform the check-and-branch
+ switch (iter()->next_bc()) {
+ case Bytecodes::_ifeq:
+ // branch if not an instance
+ do_if_helper(
+ ICmpInst::ICMP_EQ,
+ LLVMValue::jint_constant(0), result,
+ old_state, new_state);
+ break;
+
+ case Bytecodes::_ifne:
+ // branch if an instance
+ do_if_helper(
+ ICmpInst::ICMP_NE,
+ LLVMValue::jint_constant(0), result,
+ new_state, old_state);
+ break;
+
+ default:
+ ShouldNotReachHere();
}
+
+ return true;
}
-
+
void SharkTopLevelBlock::do_full_instance_check(ciKlass* klass)
{
BasicBlock *not_null = function()->CreateBlock("not_null");
@@ -1238,7 +1312,6 @@
builder()->CreateICmpEQ(object, LLVMValue::null()),
merge2, not_null);
BasicBlock *null_block = builder()->GetInsertBlock();
- SharkState *null_state = current_state()->copy();
// Get the class we're checking against
builder()->SetInsertPoint(not_null);
@@ -1282,7 +1355,6 @@
// Second merge
builder()->SetInsertPoint(merge2);
- current_state()->merge(null_state, null_block, nonnull_block);
PHINode *result = builder()->CreatePHI(
SharkType::jint_type(), "result");
result->addIncoming(LLVMValue::jint_constant(IC_IS_NULL), null_block);
diff -r 8ba2783ba3b0 -r afa643fbffde ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.hpp
--- a/ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.hpp Fri Jun 05 10:25:13 2009 -0400
+++ b/ports/hotspot/src/share/vm/shark/sharkTopLevelBlock.hpp Fri Jun 05 19:11:26 2009 +0100
@@ -358,6 +358,11 @@
// if*
private:
+ void do_if_helper(llvm::ICmpInst::Predicate p,
+ llvm::Value* b,
+ llvm::Value* a,
+ SharkState* if_taken_state,
+ SharkState* not_taken_state);
void do_if(llvm::ICmpInst::Predicate p, SharkValue* b, SharkValue* a);
// tableswitch and lookupswitch
@@ -386,11 +391,12 @@
// checkcast and instanceof
private:
- void do_optimized_instance_check();
+ bool static_subtype_check(ciKlass* check_klass, ciKlass* object_klass);
void do_full_instance_check(ciKlass* klass);
void do_trapping_instance_check(ciKlass* klass);
void do_instance_check();
+ bool maybe_do_instanceof_if();
// new and *newarray
private:
More information about the distro-pkg-dev
mailing list