Career On 27 Jan 2015 10:04, "John Rose" <john.r.r...@oracle.com> wrote:
> On Jan 26, 2015, at 8:41 AM, Vladimir Ivanov <vladimir.x.iva...@oracle.com> > wrote: > > > What do you think about the following version? > http://cr.openjdk.java.net/~vlivanov/8063137/webrev.02 > > As you suggested, I reified MHI::profileBranch on LambdaForm level and > removed @LambdaForm.Shared. My main concern about removing @Sharen was that > profile pollution can affect the code before profileBranch call (akin to > 8068915 [1]) and it seems it's the case: Gbemu (at least) is sensitive to > that change (there's a 10% difference in peak performance between @Shared > and has_injected_profile()). > > I can leave @Shared as is for now or remove it and work on the fix to the > deoptimization counts pollution. What do you prefer? > > > Generic advice here: It's better to leave it out, if in doubt. If it has > a real benefit, and we don't have time to make it clean, put it in and file > a tracking bug to clean it up. > > I re-read the change. It's simpler and more coherent now. > > I see one more issue which we should fix now, while we can. It's the sort > of thing which is hard to clean up later. > > The two fields of the profileBranch array have obscure and inconsistent > labelings. It took me some hard thought and the inspection of three files > to decide what "taken" and "not taken" mean in the C2 code that injects the > profile. The problem is that, when you look at profileBranch, all you see > is an integer (boolean) argument and an array, and no clear indication > about which array element corresponds to which argument value. It's made > worse by the fact that "taken" and "not taken" are not mentioned at all in > the JDK code, which instead wires together the branches of > selectAlternative without much comment. > > My preferred formulation, for making things clearer: Decouple the idea of > branching from the idea of profile injection. Name the intrinsic (yes, one > more bikeshed color) "profileBoolean" (or even "injectBooleanProfile"), and > use the natural indexing of the array: 0 (Java false) is a[0], and 1 (Java > true) is a[1]. We might later extend this to work with "booleans" (more > generally, small-integer flags), of more than two possible values, klasses, > etc. > > This line then goes away, and 'result' is used directly as the profile > index: > + int idx = result ? 0 : 1; > > The ProfileBooleanNode should have an embedded (or simply indirect) array > of ints which is a simple copy of the profile array, so there's no doubt > about which count is which. > > The parsing of the predicate that contains "profileBoolean" should > probably be more robust, at least allowing for 'eq' and 'ne' versions of > the test. (C2 freely flips comparison senses, in various places.) The > check for Op_AndI must be more precise; make sure n->in(2) is a constant of > the expected value (1). The most robust way to handle it (but try this > another time, I think) would be to make two temp copies of the predicate, > substituting the occurrence of ProfileBoolean with '0' and '1', > respectively; if they both fold to '0' and '1' or '1' and '0', then you > take the indicated action. > > I suggest putting the new code in Parse::dynamic_branch_prediction, which > pattern-matches for injected profiles, into its own subroutine. Maybe: > bool use_mdo = true; > if (has_injected_profile(btest, test, &taken, ¬_taken)) { > use_mdo = false; > } > if (use_mdo) { ... old code > > I see why you used the opposite order in the existing code: It mirrors > the order of the second and third arguments to selectAlternative. But the > JVM knows nothing about selectAlternative, so it's just confusing when > reading the VM code to know which profile array element means what. > > — John > > P.S. Long experience with byte-order bugs in HotSpot convinces me that if > you are not scrupulously clear in your terms, when working with equal and > opposite configuration pairs, you will have a long bug tail, especially if > you have to maintain agreement about the configurations through many layers > of software. This is one of those cases. The best chance to fix such bugs > is not to allow them in the first place. In the case of byte-order, we > have "first" vs. "second", "MSB" vs. "LSB", and "high" vs. "low" parts of > values, for values in memory and in registers, and all possible > misunderstandings about them and their relation have probably happened and > caused bugs. > > _______________________________________________ > mlvm-dev mailing list > mlvm-dev@openjdk.java.net > http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev > >
_______________________________________________ mlvm-dev mailing list mlvm-dev@openjdk.java.net http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev
