Looking very good, thanks. Ship it!
Thanks, John!
Actually, can you insert a comment why the injected counts are not scaled? (Or
perhaps they should be??)
Sure! I intentionally don't scale the counts because I don't see any
reason to do so. Profiling is done on per-MethodHandle basis, so the
counts should be very close (considering racy updates) to the actual
behavior.
Also, we may need a followup bug for the code with this comment:
// Look for the following shape: AndI (ProfileBoolean) (ConI 1))
Since profileBoolean returns a TypeInt::BOOL, the AndI with (ConI 1) should
fold up.
So there's some work to do in MulNode, which may allow that special pattern
match to go away.
But I don't want to divert the present bug by a possibly complex dive into
fixing AndI::Ideal.
Good catch! It's an overlook on my side.
The following change for ProfileBooleanNode solves the problem:
- virtual const Type *bottom_type() const { return TypeInt::INT; }
+ virtual const Type *bottom_type() const { return TypeInt::BOOL; }
I polished the change a little according to your comments (diff against
v03):
http://cr.openjdk.java.net/~vlivanov/8063137/webrev.03-04/hotspot
Changes:
- added short explanation why injected counts aren't scaled
- adjusted ProfileBooleanNode type to TypeInt::BOOL and removed
excessive pattern matching in has_injected_profile()
- added an assert when ProfileBooleanNode is removed to catch the
cases when injected profile isn't used: if we decide to generalize the
API, I'd be happy to remove it, but current usages assumes that injected
counts are always consumed during parsing and missing cases can cause
hard-to-diagnose performance problems.
Best regards,
Vladimir Ivanov
(Generally speaking, pattern matching should assume strong normalization of its inputs. Otherwise
you end up duplicating pattern match code in many places, inconsistently. Funny one-off idiom
checks like this are evidence of incomplete IR normalization. See
http://en.wikipedia.org/wiki/Rewriting for some background on terms like "normalization"
and "confluence" which are relevant to C2.)
— John
On Jan 27, 2015, at 8:05 AM, Vladimir Ivanov <vladimir.x.iva...@oracle.com>
wrote:
Thanks for the feedback, John!
Updated webrev:
http://cr.openjdk.java.net/~vlivanov/8063137/webrev.03/jdk
http://cr.openjdk.java.net/~vlivanov/8063137/webrev.03/hotspot
Changes:
- renamed MHI::profileBranch to MHI::profileBoolean, and ProfileBranchNode to
ProfileBooleanNode;
- restructured profile layout ([0] => false_cnt, [1] => true_cnt)
- factored out profile injection in a separate function
(has_injected_profile() in parse2.cpp)
- ProfileBooleanNode stores true/false counts instead of taken/not_taken
counts
- matching from value counts to taken/not_taken happens in
has_injected_profile();
- added BoolTest::ne support
- sharpened test for AndI case: now it checks AndI (ProfileBoolean) (ConI 1)
shape
Best regards,
Vladimir Ivanov
_______________________________________________
mlvm-dev mailing list
mlvm-dev@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
