Looks good.

Vladimir

Tom Rodriguez wrote:
It's updated now.

tom
On Mar 16, 2010, at 1:59 PM, Vladimir Kozlov wrote:

Yes, I was wrong, it is inserted in between. The assert is good.
But I don't see updated comment and hash_delete() calls.

Vladimir

Tom Rodriguez wrote:
On Mar 16, 2010, at 11:17 AM, Vladimir Kozlov wrote:
Tom Rodriguez wrote:
On Mar 16, 2010, at 10:46 AM, Vladimir Kozlov wrote:
I think you should use lower_bound_proj as input for upper proj:
ProjNode* upper_bound_proj = create_new_if_for_predicate(predicate_proj);
Why? That's not the usage model of create_new_if_for_predicate.
Then you have to swap next lines, otherwise upper_bound will be generated above lower_bound

2267 ProjNode* lower_bound_proj = create_new_if_for_predicate(predicate_proj);
2268 ProjNode* upper_bound_proj = create_new_if_for_predicate(predicate_proj);
create_new_if_for_predicate inserts a new if between the trap place holder if and the last created if. I'd added an assert that verified the ordering but I guess I never updated the webrev itself. It's there now and it looks like this:
+ assert(upper_bound_proj->in(0)->as_If()->in(0) == lower_bound_proj, "should dominate");
tom
Vladimir
ProjNode* PhaseIdealLoop::create_new_if_for_predicate(ProjNode* cont_proj) {
assert(is_uncommon_trap_if_pattern(cont_proj, true), "must be a uct if pattern!");
I think it would assert if I passed something else and it wouldn't improve the code shape even if I did.
Can you add to the next comment what you said in mail "late schedule may move invariant things inside the loop body"?
To remember why we need this clone.

// Perform cloning to keep Invariance state correct ok.
You should call _igvn.hash_delete() when you change boolean input for new_predicate_iff, upper_bound_iff and lower_bound_iff.
Calling hash_delete implies something which isn't true about the nodes since they are newly created and I found it confusing when I first read it which is why I removed it. I guess it would be more consistent with the other code in loop opts which pretty much always calls hash_delete. I'll restore it.
tom
Thanks,
Vladimir

Tom Rodriguez wrote:
It can have side effects because of the order that PhaseIdealLoop schedules things. It performs a late schedule so invariant things can end up inside the loop body if they are only used in the loop. The makes identifying loop members simpler but the Invariance helper class is used by the predication code to make clones outside the loop so that truly invariant values will appear invariant. So I've rearranged the code a bit more so we any values we clone are create with the control of the higher if. I've also redistributed some code which was shared before since I think it obfuscated what was really happening, particular once we needed to have two new ifs in the range check case. I'm running CTW to make sure that some of the asserts I added don't fail but it works the same as before. I've updated the webrev.
tom
On Mar 12, 2010, at 4:34 PM, Vladimir Kozlov wrote:
Fine. But as we discussed you need to verify that invar.clone() does not have any side effects.

Thanks,
Vladimir

Tom Rodriguez wrote:
That's what I get for doing edits just before generating the webrev. I'd originally deleted the clone since it seems useless. If it's invariant then cloning it doesn't change anything. I chickened out at the last minuted and restored but didn't recompile. I think I'll stick with deleting it. I've regenerated the webrev.
tom
On Mar 12, 2010, at 3:05 PM, Vladimir Kozlov wrote:
Tom,

You removed lines 2225,2226 but where ctrl node comes from for next line?:

2243 ld_rng = (LoadRangeNode*)invar.clone(ld_rng, ctrl);

Thanks,
Vladimir

Tom Rodriguez wrote:
http://cr.openjdk.java.net/~never/6930043
6930043: C2: SIGSEGV in javasoft.sqe.tests.lang.arr017.arr01702.arr01702.loop_forw(II)I
Reviewed-by:
The new loop predication code is missing logic to test that the
initial value of the index is in range. In many cases will be
eliminated statically. Tested with failing test. Also tested that
this new test doesn't affect the performance improvement we were
seeing with scimark.
src/share/vm/opto/loopTransform.cpp
test/compiler/6930043/Test6930043.java

Search Discussions

Discussion Posts

Previous

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 12 of 12 | next ›
Discussion Overview
grouphotspot-compiler-dev @
categoriesopenjdk
postedMar 12, '10 at 10:37a
activeMar 17, '10 at 10:36a
posts12
users2
websiteopenjdk.java.net

2 users in discussion

Vladimir Kozlov: 6 posts Tom Rodriguez: 6 posts

People

Translate

site design / logo © 2021 Grokbase