[PATCH] D60606: [SimpleLoopUnswitch] Implement handling of prof branch_weights metadata for SwitchInst

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:602
     DefaultExitBB = SI.getDefaultDest();
-  else if (ExitCaseIndices.empty())
+    DefaultExitWeight = SwitchInstProfUpdateWrapper::getSuccessorWeight(SI, 0);
+  } else if (ExitCaseIndices.empty())
----------------
This bit appears unnecessarily complex.  In particular, you can grab the default weight regardless of whether it's used or not later. 


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:681
   // Now add the unswitched switch.
-  auto *NewSI = SwitchInst::Create(LoopCond, NewPH, ExitCases.size(), OldPH);
+  SwitchInstProfUpdateWrapper NewSI =
+      *SwitchInst::Create(LoopCond, NewPH, ExitCases.size(), OldPH);
----------------
This is not idiomatic.  Please leave NewSI as it was, then define a separate updater object which wraps that.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:763
   // pointing at unreachable and other complexity.
   if (CommonSuccBB) {
     BasicBlock *BB = SI.getParent();
----------------
Hm, I think there's a potential reorg of the existing code which might make this easy to follow. Is there any reason we can't fully construct the switch, and then check if all successors are common?  The checking early, transform late structure just seems oddly complex for no obvious reason?


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/basictest-profmd.ll:4
+
+; This simple test would normally unswitch, but should be inhibited by the presence of
+; the noduplicate call.
----------------
If this doesn't unswitch, what is it testing?


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch-profmd.ll:6
+
+; Test for a trivially unswitchable switch with multiple exiting cases and
+; multiple looping cases.
----------------
Given the control flow, you need at least one test case where we unswitch the default only, one where we unswitch cases only, and one where we do a combination of both.  


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

https://reviews.llvm.org/D60606