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

[Kyle Butt via llvm-commits]

iteratee added inline comments.

================
Comment at: lib/CodeGen/IfConversion.cpp:703
@@ +702,3 @@
+
+  MachineBasicBlock *TT = TrueBBI.TrueBB;
+  MachineBasicBlock *TF = TrueBBI.FalseBB;
----------------
davidxl wrote:
> The variable names here does not seem to match the control flow graph drawn in the comment. Please make it consistent.
Which comment specifically? The names in the graph are member names of BBInfo, and are assumed to coincide. Here we can't make that assumption.

The names are also logical: TT = TrueBBI.TrueBB, TF = TrueBBI.FalseBB, etc.

================
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.
----------------
davidxl wrote:
> Is this a good assumption to make? Any assert can be added countDuplicatedInstructions?
I've elaborated in the comment. The size is computed by ScanInstructions, and the duplicated portion is subtracted off, so there's no point in recomputing the size, we would get the same answer.

================
Comment at: lib/CodeGen/IfConversion.cpp:781
@@ +780,3 @@
+    // non-duplicated section.
+    if (BBIRecalc.ClobbersPred)
+      return false;
----------------
davidxl wrote:
> Why is this check not done outside of this function (in ValidForkedForkedDiamond before countDuplicatedInstuctions as in ValidDiamond ?
Because countDuplicatedInstructions adjusts the iterators so that we know exactly which instructions are duplicated. We only worry about the non-duplicated instructions that clobber the predicate info.

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

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

================
Comment at: lib/CodeGen/IfConversion.cpp:1116
@@ +1115,3 @@
+    BBInfo TrueBBICalc, FalseBBICalc;
+    if (CanRevCond && ValidForkedDiamond(TrueBBI, FalseBBI, Dups, Dups2,
+                                         TrueBBICalc, FalseBBICalc) &&
----------------
davidxl wrote:
> 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);
> 
> 
It would take too many parameters, and the code would be less legible.

I would have to pass in TrueBBICalc, FalseBBICalc, hasCommonTail, and then conditionalize the calls, because
ValidDiamond takes a different number of arguments from ValidForkedDiamond.

I don't think it would be cleaner.


https://reviews.llvm.org/D22551