[PATCH] D66190: [CodeGen] Add a pass to do block predication on SSA machine IR

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:930-932
+//===----------------------------------------------------------------------===//
+//                           EarlyIfPredicator Pass
+//===----------------------------------------------------------------------===//
----------------
lebedev.ri wrote:
> ThomasRaoux wrote:
> > lebedev.ri wrote:
> > > ThomasRaoux wrote:
> > > > lebedev.ri wrote:
> > > > > I'd think you want to not jam it next to existing pass, but put it into it's own place
> > > > Do you mean move it into its own file? This reuses SSAIfConv infrastructure so unless we move it into a header file those two passes have to be in the same file.
> > > Yes
> > So you are suggesting moving SSAIfConv into a common header? I don't have a strong opinion about it but there are several cases of passes sharing a file and I suppose limiting exposure of the helper has its advantage.
> > So you are suggesting moving SSAIfConv into a common header?
> Yes.
> I'm also wondering what would be the best structure for that class,
> should it do both things, or should it be generalized somehow,
> and have two classes that inherit form it (to cmov, to branches)
> that do the actual work.
FWIW I don't think this should be a concern. Moving code for the sake of moving code doesn't sound particularly useful to me. I see this as analogous to MachineScheduler and PostMachineScheduler which share common infrastructure and also the same file.

> I'm also wondering what would be the best structure for that class

This current structure looks fine to me. Roman, if you have suggestions for how you'd like the code to look, perhaps you could  mention them explicitly? your current comment doesn't feel particularly actionable.



================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:296
+  // get right.
+  if (!MBB->livein_empty()) {
+    LLVM_DEBUG(dbgs() << printMBBReference(*MBB) << " has live-ins.\n");
----------------
I'm not sure we should be so concerned with this case for predication, but it's sensible to keep this in for now. Erring on the side of caution is good.


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:323
+
+    // Check that instruction is predicatable and that it is not already
+    // predicated.
----------------
s/predicatable/predicable/


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:339
+  auto Condition = Cond;
+  if (ReversePredicate) {
+    TII->reverseBranchCondition(Condition);
----------------
nit: General LLVM style is to elide braces around single statements.


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:637
+    if (Predicate)
+      PredicateBlock(TBB, /*ReversePredicate*/ false);
     Head->splice(InsertionPoint, TBB, TBB->begin(), TBB->getFirstTerminator());
----------------
nit: /*ReversePredicate=*/false (add =, remove space, for standard comment style that clang-tidy can pick up).


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:972
+INITIALIZE_PASS_END(EarlyIfPredicator, DEBUG_TYPE,
+                    "Early If Predicatir", false, false)
+
----------------
typo.


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:1057
+    Changed = true;
+    updateDomTree(RemovedBlocks);
+    updateLoops(RemovedBlocks);
----------------
For my own gratification, could you please comment (in the review, not the code) why this is required for predication but not for speculation?


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

https://reviews.llvm.org/D66190