[PATCH] D34109: [AArch64] Update successor probabilities after ccmp-conversion

Matthew Simpson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 11:40:20 PDT 2017


mssimpso added a comment.

Thanks for the comments Matthias! Responses are inline.



================
Comment at: lib/Target/AArch64/AArch64ConditionalCompares.cpp:143
   MachineRegisterInfo *MRI;
+  const MachineBranchProbabilityInfo *MBPI;
 
----------------
MatzeB wrote:
> Huh, we can use a const instance of this to change the probabilities... Looking at MachineBranchProbabilityInfo I wonder why this was implemented as a pass and not just a set of utility functions. Anyway just wanted to write this down, it isn't really relevant for the review at hand.
I agree, this was a bit confusing to me. My understanding is that `MachineBranchProbabilityInfo` is required for reading off the successor probabilities, but that the setting of the probabilities must be done via `MachineBasicBlock`.


================
Comment at: lib/Target/AArch64/AArch64ConditionalCompares.cpp:590
+    BranchProbability Head2Tail = MBPI->getEdgeProbability(Head, Tail);
+    Head->setSuccProbability(Head->succ_begin(),
+                             Head2Tail + Head2CmpBB * CmpBB2Tail);
----------------
MatzeB wrote:
> I cannot convince myself right now that it is always `succ_begin()` that points to `Tail`. Wouldn't we need to search for the right successor?
`Head` is required to have exactly two successors, `CmpBB` and `Tail` (see the beginning part of `canConvert`). `CmpBB` is removed as a successor of `Head` a few lines up (577), so `succ_begin()` can only point to `Tail` here.


https://reviews.llvm.org/D34109





More information about the llvm-commits mailing list