[PATCH] D60769: [SimpleLoopUnswitch] Discard stale prof branch_weights for partial unswitched branches

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

The change description and your tests make it hard to determine what your intended result is.  Please fix.

Please update your change description to explain *why* stripping metadata is the best answer.  A small example might help.  Or alternatively, turn the small examples into (new) test cases and reference them from the proposed commit message.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:490
                                           *UnswitchedBB, *NewPH);
+    // If there is any prof branch_weights metadata then discard them because
+    // they are no longer valid for the split condition.
----------------
I believe the same problem exists for full unswitch as well, and thus this change should not be conditional.

As an example, consider:
loop {
  if (A) break
  if (B) break;
}

Where we're unswitching B, but not A for some reason.  The frequency on the new B branch could be quite different depending on whether A and B are correlated.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:2013
+  // condition.
+  if (!FullUnswitch)
+    BI->setMetadata(LLVMContext::MD_prof, nullptr);
----------------
I'd suggest moving this to right after the branch successor rewriting.

Also, there is a bug here if this function is called w/BI null and SI non null.


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch.ll:2703
 ; CHECK-LABEL: @test25(
 entry:
   br label %loop_begin
----------------
Please separate and commit your test changes, then rebase over them.  At the moment, I can't actually tell what's changing vs what are changes to the tests.

note that you should make sure to have tests both with and without profile weights.  You don't need to (and should not) fully duplicate, but at least a few of each are required.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60769/new/

https://reviews.llvm.org/D60769