[PATCH] D22551: CodeGen: If Convert blocks that would form a diamond when tail-merged.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/IfConversion.cpp:703
@@ +702,3 @@
+
+  MachineBasicBlock *TT = TrueBBI.TrueBB;
+  MachineBasicBlock *TF = TrueBBI.FalseBB;
----------------
The variable names here does not seem to match the control flow graph drawn in the comment. Please make it consistent.

================
Comment at: lib/CodeGen/IfConversion.cpp:740
@@ +739,3 @@
+
+  // TIE and FIE both point at the last instruction, move them back.
+  ++TIE; ++FIE;
----------------
Clean up the comment -- last instruction of what?

================
Comment at: lib/CodeGen/IfConversion.cpp:741
@@ +740,3 @@
+  // TIE and FIE both point at the last instruction, move them back.
+  ++TIE; ++FIE;
+  // The size of the blocks are the same.
----------------
Is this a good assumption to make? Any assert can be added countDuplicatedInstructions?

================
Comment at: lib/CodeGen/IfConversion.cpp:773
@@ +772,3 @@
+    // Skip dbg_value instructions. These do not count.
+    if (BIB->isDebugValue()) {
+      while (BIB != BIE && BIB->isDebugValue())
----------------
This skip code pattern has appeared many times -- good candidate to extract into an inline function.

================
Comment at: lib/CodeGen/IfConversion.cpp:781
@@ +780,3 @@
+    // non-duplicated section.
+    if (BBIRecalc.ClobbersPred)
+      return false;
----------------
Why is this check not done outside of this function (in ValidForkedForkedDiamond before countDuplicatedInstuctions as in ValidDiamond ?

================
Comment at: lib/CodeGen/IfConversion.cpp:786
@@ +785,3 @@
+    if (TII->DefinesPredicate(*BIB, PredDefs))
+      BBIRecalc.ClobbersPred = true;
+    if (BIB->isBranch())
----------------
Why is this not already computed?

================
Comment at: lib/CodeGen/IfConversion.cpp:789
@@ +788,3 @@
+      return false;
+    if (!TII->isPredicable(*BIB))
+      return false;
----------------
feasbilityAnalysis already checks isUnpredicable bit -- why is it still done here?

================
Comment at: lib/CodeGen/IfConversion.cpp:1116
@@ +1115,3 @@
+    BBInfo TrueBBICalc, FalseBBICalc;
+    if (CanRevCond && ValidForkedDiamond(TrueBBI, FalseBBI, Dups, Dups2,
+                                         TrueBBICalc, FalseBBICalc) &&
----------------
This code looks almost exactly the same as the regular diamond case. Perhaps defined a lamba function 
auto DiamondFinder = [&](decltype(&IfConverter::ValidDiamond) Checker) {
     if (CanRevCond && (this->*Checker(..) && ...) {
    ....
   }
};

DiamondFinder(&IfConverter::ValidDiamond);
DiamondFinder(&IfConverter::ValidForkedDiamond);




https://reviews.llvm.org/D22551