8325746: Refactor Loop Unswitching code #17842

chhagedorn · 2024-02-14T09:06:03Z

To simplify the review of #16877, I've decided to split off some code into separate sub-tasks.

This sub-task focuses on the code refactoring done in Loop Unswitching. I've incorporated the feedback from @eme64 at #16877 in the proposed changes.

Changes

Adding a more detailed description about the Loop Unswitching optimization itself below the file header.
Renaming fast loop -> true-path-loop and slow loop -> false-path-loop since we don't really know which one is faster and to avoid keeping a mental map of which path goes into which unswitched loop version.
Creating new classes:
- UnswitchedLoopSelector to create the unswitched loop selector If (i.e. the If node that selects at runtime which unswitched loop version is executed) based on the invariant and non-loop-exiting unswitch candidate If.
- OriginalLoop to do the actual unswitching (i.e. loop cloning, predicates cloning and fixing the graph to the loop selector If).
Extracting methods in do_unswitching() to clean it up further.
Improving TraceLoopUnswitching result printing.
Adding some shared utility methods.

Possible Follow-Up RFEs

There are possible follow-up opportunities to further improve the loop unswitching which, however, would go to far in the context of fixing Assertion Predicates. We could follow up with RFEs to tackle these:

At some point it might be good to have a dedicated LoopUnswitching class to split all the methods off from PhaseIdealLoop. This would also avoid noisy "loop_unswitching" prefixes.
Bailout code in has_control_dependencies_from_predicates() is too conservative. I have already some code to improve that in the full fix for Assertion Predicates which I plan to split off and propose separatly.
TraceLoopUnswitching currently only prints the result and if it failed. We might want to improve that in the future to cover more steps to make it useful for debugging (would require some more fine-grained debug levels, though).

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8325746: Refactor Loop Unswitching code (Sub-task - P4)

Reviewers

Emanuel Peter (@eme64 - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17842/head:pull/17842
$ git checkout pull/17842

Update a local copy of the PR:
$ git checkout pull/17842
$ git pull https://git.openjdk.org/jdk.git pull/17842/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17842

View PR using the GUI difftool:
$ git pr show -t 17842

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17842.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-14T09:06:48Z

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-14T09:07:15Z

@chhagedorn The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

chhagedorn · 2024-02-14T09:09:52Z

src/hotspot/share/opto/loopUnswitch.cpp

-  if (TraceLoopOpts) {
-    tty->print("Unswitch   %d ", head->unswitch_count()+1);
-    loop->dump_head();


Extracted to trace_loop_unswitching_count()

chhagedorn · 2024-02-14T09:10:12Z

src/hotspot/share/opto/loopUnswitch.cpp

-  if (head->is_CountedLoop() && !head->as_CountedLoop()->is_normal_loop()) {
-    head->as_CountedLoop()->set_normal_loop();


Extracted to revert_to_normal_loop()

chhagedorn · 2024-02-14T09:10:39Z

src/hotspot/share/opto/loopUnswitch.cpp

+  LoopNode* original_head = loop->_head->as_Loop();
+  if (has_control_dependencies_from_predicates(original_head)) {
+    NOT_PRODUCT(trace_loop_unswitching_impossible(original_head);)
+    return;
  }


Extracted to hoist_invariant_check_casts()

chhagedorn · 2024-02-14T09:11:01Z

src/hotspot/share/opto/loopUnswitch.cpp

-  LoopNode* head_clone = old_new[head->_idx]->as_Loop();
-  int nct = head->unswitch_count() + 1;
-  head->set_unswitch_count(nct);
-  head_clone->set_unswitch_count(nct);


Extracted to increment_unswitch_counts().

chhagedorn · 2024-02-14T09:11:39Z

src/hotspot/share/opto/loopUnswitch.cpp

-  // Reoptimize loops
-  loop->record_for_igvn();
-  for(int i = loop->_body.size() - 1; i >= 0 ; i--) {
-    Node *n = loop->_body[i];
-    Node *n_clone = old_new[n->_idx];
-    _igvn._worklist.push(n_clone);
-  }


Extracted to add_unswitched_loop_version_bodies_to_igvn()

chhagedorn · 2024-02-14T09:11:57Z

src/hotspot/share/opto/loopUnswitch.cpp

-#ifndef PRODUCT
-  if (TraceLoopUnswitching) {
-    tty->print_cr("Loop unswitching orig: %d @ %d  new: %d @ %d",
-                  head->_idx,                unswitch_iff->_idx,
-                  old_new[head->_idx]->_idx, unswitch_iff_clone->_idx);
-  }
-#endif


Extracted to trace_loop_unswitching_result()

chhagedorn · 2024-02-14T09:13:09Z

src/hotspot/share/opto/loopUnswitch.cpp

-  IfNode* invar_iff = create_slow_version_of_loop(loop, old_new, unswitch_iff, CloneIncludesStripMined);
-  ProjNode* proj_true = invar_iff->proj_out(1);
-  verify_fast_loop(head, proj_true);


Refactored to create_unswitched_loop_versions().

chhagedorn · 2024-02-14T09:13:40Z

src/hotspot/share/opto/loopUnswitch.cpp

-  _igvn.rehash_node_delayed(unswitch_iff);
-  dominated_by(proj_true->as_IfProj(), unswitch_iff);
+  NOT_PRODUCT(trace_loop_unswitching_count(loop, original_head);)
+  C->print_method(PHASE_BEFORE_LOOP_UNSWITCHING, 4, original_head);

-  IfNode* unswitch_iff_clone = old_new[unswitch_iff->_idx]->as_If();
-  _igvn.rehash_node_delayed(unswitch_iff_clone);
-  ProjNode* proj_false = invar_iff->proj_out(0);
-  dominated_by(proj_false->as_IfProj(), unswitch_iff_clone);


Extracted to remove_unswitch_candidate_from_loops() and put inside create_unswitched_loop_versions().

src/hotspot/share/opto/loopopts.cpp

mlbridge · 2024-02-14T09:22:28Z

Webrevs

eme64

Nice work! A first batch of comments.

src/hotspot/share/opto/ifnode.cpp

eme64 · 2024-02-14T09:38:47Z

src/hotspot/share/opto/loopUnswitch.cpp

+// Loop Unswitching is a loop optimization to move an invariant, non-loop-exiting test in the loop body before the loop.
+// This is done by duplicating the loop and placing the if-path in one unswitched loop version (i.e. the true-path-loop)
+// and the else-path in the other (i.e. the false-path-loop). The invariant test only needs to be checked once and
+// depending on the outcome, one or the other unswitched loop version is executed:


Suggested change

// Loop Unswitching is a loop optimization to move an invariant, non-loop-exiting test in the loop body before the loop.

// This is done by duplicating the loop and placing the if-path in one unswitched loop version (i.e. the true-path-loop)

// and the else-path in the other (i.e. the false-path-loop). The invariant test only needs to be checked once and

// depending on the outcome, one or the other unswitched loop version is executed:

// Loop Unswitching is a loop optimization to move an invariant, non-loop-exiting test in the loop body before the loop.

// For one loop execution, this test is either always true or always false. Hence, we duplicate the loop: the true-path-loop always takes the true-path and the false-path-loop always takes the false-path. We move the test

// before the loops, turning it into a loop selector. The true-proj leads to the true-path-loop, and the false-proj

// leads to the false-path-loop. Now the invariant test is only checked once before the loop, and decides which

// unswitched loop version is executed:

Make sure to be consistent. I think you use if-path and if-block as synonymes, right?
Btw: are these only blocks, as in basic blocks, and hence the if-statements are only such "basic diamonds", that only have one block per if-else side? Or could the CFG be more complex?

placing the if-path in one unswitched loop version
That is a bit confusing. It sounds like you are taking the hoisted if-true-projection, and putting it inside a loop, which does not make sense.

later you name the hoisted if a "loop selector". We could already name it the same here.

BTW: why do we not hoist exit conditions also? Would that not also be profitable? Just out of curiosity...

BTW: why do we not hoist exit conditions also? Would that not also be profitable? Just out of curiosity...

This can better be done with a simple loop peeling which can hoist the invariant checks. See:

jdk/src/hotspot/share/opto/loopTransform.cpp

Lines 499 to 502 in 0c2def0

// Condition is not a member of this loop?

if (!is_member(phase->get_loop(ctrl)) && is_loop_exit(test)) {

return estimate; // Found reason to peel!

}

Ah yes, makes sense!
Maybe add a small comment about loop peeling then? Just because you do mention that we don't handle exits, that automatically raises that question.

Ah yes, makes sense! Maybe add a small comment about loop peeling then? Just because you do mention that we don't handle exits, that automatically raises that question.

I'll add a comment.

Make sure to be consistent. I think you use if-path and if-block as synonymes, right?
Btw: are these only blocks, as in basic blocks, and hence the if-statements are only such "basic diamonds", that only have one block per if-else side? Or could the CFG be more complex?

The only requirement for Loop Unswitching is that both paths are inside the loop (I make this more explicit in the header comment together with mentioning loop peeling as alternative for loop-exiting tests). When this is given, we will at some point merge both paths again with a region. So, yes, I use if-path and if-block interchangeable. I've cleaned that up in the comments.

For example, this example can also be unswitched on flag with a more complex CFG and even with a method call:

static int iFld; static boolean flag2; // Run with -Xcomp. static void test() { boolean flag = iFld < 3; for (int i = 0; i < 1000; i++) { if (flag) { iFld = 34; if (flag2) { dontInline(); } } } }

I've rewritten this paragraph inspired by your suggestions. Please have another look at it :-)

src/hotspot/share/opto/loopUnswitch.cpp

eme64

Nice Work @chhagedorn . I like that you packed things into classes. It nicely packs things into more digestable concepts. And allows you to create objects of those classes, and then pass then around as const, after which the reader knows they are not changed anymore. Very helpful!

src/hotspot/share/opto/loopUnswitch.cpp

eme64 · 2024-02-14T10:45:10Z

src/hotspot/share/opto/loopUnswitch.cpp

+}
+
+// Remove the unswitch candidate If nodes in both unswitched loop versions which are now dominated by the loop selector
+// If node. Keep the true path block in the true-path-loop and the false path block in the false-path-loop by setting


As elsewhere: is it really right to talk about "blocks"? Or could the CFG be more complicated?

src/hotspot/share/opto/loopUnswitch.cpp

src/hotspot/share/opto/loopopts.cpp

src/hotspot/share/opto/loopUnswitch.cpp

eme64 · 2024-02-14T11:12:05Z

src/hotspot/share/opto/loopUnswitch.cpp

 #endif // ASSERT

+ public:
+  // Unswitch on the invariant loop selector by creating two unswitched loop versions.
+  void unswitch(const UnswitchedLoopSelector& unswitched_loop_selector) {


Just a suggestion, I am also ok with what it is now:
I wonder if the name unswitch is right here. Because at this point you only clone the loops, you don't yet remove the if. Maybe this should for now just be called "duplicate", and create_unswitched_loop_versions then really does the unswitching, by both duplicating and removing the ifs?

I've moved remove_unswitch_candidate_from_loop() into this method to really have the complete unswitching process in this method. This also allowed to get rid of create_unswitched_loop_versions() which now was only a redundant delegation. This required to move the OriginalLoop class further up.

eme64 · 2024-02-14T11:14:49Z

src/hotspot/share/opto/loopUnswitch.cpp

+class OriginalLoop : public StackObj {
+  LoopNode* const _strip_mined_loop_head;
+  IdealLoopTree* const _loop;
+  Node_List* const _old_new;


Could also be held "by reference". Then you don't have convert it from and to pointer. Well, idk, maybe it would be just as bad the other way around.

eme64 · 2024-02-14T11:18:39Z

src/hotspot/share/opto/loopUnswitch.cpp

+void PhaseIdealLoop::revert_to_normal_loop(const LoopNode* loop_head) {
+  if (loop_head->is_CountedLoop() && !loop_head->as_CountedLoop()->is_normal_loop()) {
+    loop_head->as_CountedLoop()->set_normal_loop();
+  }


Suggested change

void PhaseIdealLoop::revert_to_normal_loop(const LoopNode* loop_head) {

if (loop_head->is_CountedLoop() && !loop_head->as_CountedLoop()->is_normal_loop()) {

loop_head->as_CountedLoop()->set_normal_loop();

}

void PhaseIdealLoop::revert_to_normal_loop(const LoopNode* loop_head) {

CountedLoopNode* cl = loop_head->isa_CountedLoop();

if (cl != nullptr && !cl->is_normal_loop()) {

cl->set_normal_loop();

}

Would make the code easier to read

I just moved the code but I can clean it up as you suggested.

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

chhagedorn

Thanks a lot for your suggestions! I've pushed updates addressing them all.

src/hotspot/share/opto/loopUnswitch.cpp

chhagedorn · 2024-02-14T12:59:28Z

src/hotspot/share/opto/loopUnswitch.cpp

+// Loop Unswitching is a loop optimization to move an invariant, non-loop-exiting test in the loop body before the loop.
+// This is done by duplicating the loop and placing the if-path in one unswitched loop version (i.e. the true-path-loop)
+// and the else-path in the other (i.e. the false-path-loop). The invariant test only needs to be checked once and
+// depending on the outcome, one or the other unswitched loop version is executed:


Ah yes, makes sense! Maybe add a small comment about loop peeling then? Just because you do mention that we don't handle exits, that automatically raises that question.

I'll add a comment.

Make sure to be consistent. I think you use if-path and if-block as synonymes, right?
Btw: are these only blocks, as in basic blocks, and hence the if-statements are only such "basic diamonds", that only have one block per if-else side? Or could the CFG be more complex?

The only requirement for Loop Unswitching is that both paths are inside the loop (I make this more explicit in the header comment together with mentioning loop peeling as alternative for loop-exiting tests). When this is given, we will at some point merge both paths again with a region. So, yes, I use if-path and if-block interchangeable. I've cleaned that up in the comments.

For example, this example can also be unswitched on flag with a more complex CFG and even with a method call:

static int iFld; static boolean flag2; // Run with -Xcomp. static void test() { boolean flag = iFld < 3; for (int i = 0; i < 1000; i++) { if (flag) { iFld = 34; if (flag2) { dontInline(); } } } }

chhagedorn · 2024-02-14T13:32:27Z

src/hotspot/share/opto/loopUnswitch.cpp

+// Loop Unswitching is a loop optimization to move an invariant, non-loop-exiting test in the loop body before the loop.
+// This is done by duplicating the loop and placing the if-path in one unswitched loop version (i.e. the true-path-loop)
+// and the else-path in the other (i.e. the false-path-loop). The invariant test only needs to be checked once and
+// depending on the outcome, one or the other unswitched loop version is executed:


I've rewritten this paragraph inspired by your suggestions. Please have another look at it :-)

src/hotspot/share/opto/loopUnswitch.cpp

chhagedorn · 2024-02-14T13:47:15Z

src/hotspot/share/opto/loopUnswitch.cpp

+class OriginalLoop : public StackObj {
+  LoopNode* const _strip_mined_loop_head;
+  IdealLoopTree* const _loop;
+  Node_List* const _old_new;


chhagedorn · 2024-02-14T13:58:23Z

src/hotspot/share/opto/loopUnswitch.cpp

 #endif // ASSERT

+ public:
+  // Unswitch on the invariant loop selector by creating two unswitched loop versions.
+  void unswitch(const UnswitchedLoopSelector& unswitched_loop_selector) {


I've moved remove_unswitch_candidate_from_loop() into this method to really have the complete unswitching process in this method. This also allowed to get rid of create_unswitched_loop_versions() which now was only a redundant delegation. This required to move the OriginalLoop class further up.

src/hotspot/share/opto/loopUnswitch.cpp

eme64 · 2024-02-14T14:20:30Z

src/hotspot/share/opto/loopUnswitch.cpp

+//        if-block                    =========>              [Cloned Template                  [Cloned Template
+//      else                                                   Assertion Predicates]             Assertion Predicates]
+//        // could be empty                                         |                                  |
+//        [else-block]                                        True-Path-Loop                    False-Path-Loop


Could it also be the other way around, that the if-path is empty but the else-path has code?

Yes definitely, but this is more a reflection of Java code where you always have an if block but not always an else block. But it might be cleaner to just remove the "could be empty" comment and make it explicit and not worry about whether one block is empty. I'll do that. The image should just give a hint :-)

eme64

Thanks for the updates, we are almost there :)

src/hotspot/share/opto/loopUnswitch.cpp

eme64 · 2024-02-14T14:24:21Z

src/hotspot/share/opto/loopUnswitch.cpp

+//      stmt2                                                   cloned stmt1                      cloned stmt1
+//    Endloop                                                   cloned if-path                    [cloned else-path]
+//                                                              cloned stmt2                      cloned stmt2
+//                                                            Endloop                           Endloop


Could it be the other way around: the if-path is empty and the else-path has code? Or are you just trying to suggest that there could be an empty path, and that this would really simplify one of the cloned loops now?

Yes, it would be a simplification. But as suggested in the other comment. It might create more confusion than actually helping.

src/hotspot/share/opto/loopUnswitch.cpp

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

vnkozlov · 2024-02-14T18:52:48Z

src/hotspot/share/opto/loopnode.hpp

@@ -770,6 +771,11 @@ class IdealLoopTree : public ResourceObj {
    return _has_range_checks;
  }

+  // Return the strip mined loop tree if there is an outer strip mined loop. Otherwise, return this.


The comment is not clear for me. Something like next?:
"// Return parent's IdealLoopTree for strip mined loop and this for other cases."

vnkozlov

Good refactoring. My main complain is long comments which are difficult to read if you edit file in terminal.

vnkozlov · 2024-02-14T18:55:21Z

src/hotspot/share/opto/loopUnswitch.cpp

+// - Original loop -> true-path-loop: The true-path of the invariant, non-loop-exiting test in the original loop
+//                                    is kept while the false-path is killed. We call this unswitched loop version
+//                                    the true-path-loop.
+// - Cloned loop -> False-path-loop:  The false-path of the invariant, non-loop-exiting test in the cloned loop
+//                                    is kept while the true-path is killed. We call this unswitched loop version
+//                                    the false-path loop.


Comments are too wide and difficult to read. Suggestion is to put explanation on new line with shift:

// - Original loop -> true-path-loop: // The true-path of the invariant, non-loop-exiting test in the original loop // is kept while the false-path is killed. We call this unswitched loop version // the true-path-loop.

vnkozlov · 2024-02-14T18:57:03Z

src/hotspot/share/opto/loopUnswitch.cpp

+//    Endloop                                                   cloned stmt1                      cloned stmt1
+//                                                              cloned if-path                    cloned else-path
+//                                                              cloned stmt2                      cloned stmt2
+//                                                            Endloop                           Endloop


Can you put resulting unstitched graph on separate following lines? Comment line are too long.

chhagedorn · 2024-02-15T08:46:49Z

Thanks @eme64 and @vnkozlov for your reviews! I've addressed your latest comments.

About the long lines: I'm using 120 chars by default which fits nicely on the screen in the IDE and is also the default suggested by the IDE. But I guess that's also a matter of taste. Unfortunately, our style guide also does not mention anything about it - maybe because it's a personal preference. But anyway, I agree that some comments were a bit too densely packed. I've updated them according to your suggestions. Let me know if there are other comments that you think should be updated as well.

vnkozlov

Much better.

openjdk · 2024-02-15T21:45:27Z

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8325746: Refactor Loop Unswitching code

Reviewed-by: epeter, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 176 new commits pushed to the master branch:

9f0e7da: 8326638: Crash in PhaseIdealLoop::remix_address_expressions due to unexpected Region instead of Loop
81b065a: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c
4fcae1a: 8326722: Cleanup unnecessary forward declaration in collectedHeap.hpp
c5c866a: 8326219: applications/kitchensink/Kitchensink8H.java timed out
ac3ce2a: 8326583: Remove over-generalized DefineNativeToolchain solution
bceaed6: 8326406: Remove mapfile support from makefiles
16d917a: 8313710: jcmd: typo in the documentation of JFR.start and JFR.dump
60cbf29: 8325754: Dead AbstractQueuedSynchronizer$ConditionNodes survive minor garbage collections
da14aa4: 8017234: Hotspot should stop using mapfiles
d482c1a: 8326524: Rename agent_common.h
... and 166 more: https://git.openjdk.org/jdk/compare/6b7c9718d68f30f47a163042d6e205945b9ff365...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

eme64

Nice work, Christian!

src/hotspot/share/opto/loopUnswitch.cpp

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

chhagedorn · 2024-02-16T08:14:54Z

Thanks Vladimir and Emanuel for your reviews and also Emanuel for the offline discussion!

chhagedorn · 2024-02-16T08:15:15Z

I'll resubmit some testing again before integration.

chhagedorn · 2024-02-26T07:20:57Z

Testing showed that make_with_same_profile() could also be called with a BaseCountedEndLoopNode (attached extracted test for int and long). We could be unswitching on a BaseCountedEndLoopNode after peeling one iteration - the exit test then serves as a zero trip guard. I've changed the code accordingly to fall back to IfNode if it's not a RangeCheckNode. I've also changed the assertion to allow all these nodes but not a ParsePredicateNode on which make_with_same_profile() should never be called.

eme64

Update makes sense. A few suggestions below.

eme64 · 2024-02-26T09:55:00Z

test/hotspot/jtreg/compiler/loopopts/TestBaseCountedEndLoopUnswitchCandidate.java

+ * @requires vm.compiler2.enabled
+ * @run main/othervm -XX:CompileCommand=compileonly,compiler.loopopts.TestBaseCountedEndLoopUnswitchCandidate::test*
+ *                   -Xcomp -XX:LoopMaxUnroll=0 -XX:-UseLoopPredicate -XX:-RangeCheckElimination
+ *                   compiler.loopopts.TestBaseCountedEndLoopUnswitchCandidate


Might it make sense to have a run with fewer flags?

Do you mean an additional run? For the current run, we require the C2 flags in order to have loop peeling -> loop unswitching to get unswitching on a BaseCountedLoopEndNode. We might be able to find a test that requires fewer flags but I think it's okay to opt for a simpler test + flags.

Yes, I thought you could just add another @run with fewer flags, so that external flags have more influence, and potentially trigger other cases.

Sounds good, added such a run.

eme64 · 2024-02-26T09:56:23Z

src/hotspot/share/opto/ifnode.cpp

+    // the same profiling if that actually makes sense. Some If node subtypes should not be cloned in this way.
+    // In theory, we should not clone BaseCountedLoopEndNodes. But they can end up being used as normal If nodes when
+    // peeling a loop - they serve as zero-trip guard. Allow them as well.
+    assert(if_node_profile->Opcode() == Op_If || if_node_profile->is_RangeCheck()


if_node_profile->is_RangeCheck() seems excluded by the if-else above...
You could move the assert before the if, then it would be correct, I think

eme64 · 2024-02-26T09:57:27Z

test/hotspot/jtreg/compiler/loopopts/TestBaseCountedEndLoopUnswitchCandidate.java

+    }
+
+
+    public static void testLongCountedLoopEnd() {


Remove a newline

chhagedorn

Thanks Emanuel for your re-review! Pushed an update.

src/hotspot/share/opto/loopUnswitch.cpp

chhagedorn · 2024-02-26T11:28:20Z

test/hotspot/jtreg/compiler/loopopts/TestBaseCountedEndLoopUnswitchCandidate.java

+ * @requires vm.compiler2.enabled
+ * @run main/othervm -XX:CompileCommand=compileonly,compiler.loopopts.TestBaseCountedEndLoopUnswitchCandidate::test*
+ *                   -Xcomp -XX:LoopMaxUnroll=0 -XX:-UseLoopPredicate -XX:-RangeCheckElimination
+ *                   compiler.loopopts.TestBaseCountedEndLoopUnswitchCandidate


Do you mean an additional run? For the current run, we require the C2 flags in order to have loop peeling -> loop unswitching to get unswitching on a BaseCountedLoopEndNode. We might be able to find a test that requires fewer flags but I think it's okay to opt for a simpler test + flags.

8325746: Refactor Loop Unswitching code

4fd6d19

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 14, 2024

chhagedorn commented Feb 14, 2024

View reviewed changes

chhagedorn marked this pull request as ready for review February 14, 2024 09:18

openjdk bot added the rfr Pull request is ready for review label Feb 14, 2024

eme64 suggested changes Feb 14, 2024

View reviewed changes

chhagedorn and others added 4 commits February 14, 2024 12:45

Apply Emanuel's suggestions

f73c5fb

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Apply suggestions from code review

17c27a5

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Update src/hotspot/share/opto/loopUnswitch.cpp

2e0d2a3

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

Suggestions by Emanuel

40dbdd8

chhagedorn commented Feb 14, 2024

View reviewed changes

Change block -> path

66532d2

eme64 reviewed Feb 14, 2024

View reviewed changes

eme64 suggested changes Feb 14, 2024

View reviewed changes

chhagedorn and others added 2 commits February 14, 2024 15:48

fix empty path, strip mined loop head

574eb60

Apply suggestions from code review

cbcf3dc

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

vnkozlov reviewed Feb 14, 2024

View reviewed changes

Vladimir's review

9b1d3f1

chhagedorn mentioned this pull request Feb 15, 2024

8305638: Renaming and small clean-ups around predicates #16877

Closed

3 tasks

vnkozlov approved these changes Feb 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 15, 2024

eme64 approved these changes Feb 16, 2024

View reviewed changes

src/hotspot/share/opto/loopUnswitch.cpp Outdated Show resolved Hide resolved

Update src/hotspot/share/opto/loopUnswitch.cpp

f685395

Co-authored-by: Emanuel Peter <emanuel.peter@oracle.com>

eme64 approved these changes Feb 16, 2024

View reviewed changes

Fix If creation and add assert with test

5a314ff

eme64 approved these changes Feb 26, 2024

View reviewed changes

Review Emanuel

96798d1

chhagedorn commented Feb 26, 2024

View reviewed changes

Add additional run

		if (head->is_CountedLoop() && !head->as_CountedLoop()->is_normal_loop()) {
		head->as_CountedLoop()->set_normal_loop();

	// Condition is not a member of this loop?
	if (!is_member(phase->get_loop(ctrl)) && is_loop_exit(test)) {
	return estimate; // Found reason to peel!
	}

8325746: Refactor Loop Unswitching code #17842

8325746: Refactor Loop Unswitching code #17842

Conversation

chhagedorn commented Feb 14, 2024 • edited by openjdk bot Loading

Changes

Possible Follow-Up RFEs

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 14, 2024

openjdk bot commented Feb 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Feb 14, 2024 • edited Loading

Webrevs

eme64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eme64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eme64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn commented Feb 15, 2024 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 15, 2024 • edited Loading

eme64 left a comment

Choose a reason for hiding this comment

chhagedorn commented Feb 16, 2024

chhagedorn commented Feb 16, 2024

chhagedorn commented Feb 26, 2024

eme64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn left a comment

Choose a reason for hiding this comment

chhagedorn Feb 26, 2024 • edited Loading

chhagedorn commented Feb 14, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 14, 2024 •

edited

Loading

chhagedorn Feb 14, 2024 •

edited

Loading

chhagedorn commented Feb 15, 2024 •

edited

Loading

openjdk bot commented Feb 15, 2024 •

edited

Loading

chhagedorn Feb 26, 2024 •

edited

Loading

chhagedorn Feb 26, 2024 •

edited

Loading