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

[David Li via llvm-commits]

davidxl added a comment.

The code looks in pretty good shape now.   I find the test case is little missing -- how about adding some more (including negative one)?


================
Comment at: lib/CodeGen/IfConversion.cpp:90
@@ -86,1 +89,3 @@
+      ICForkedDiamond  // BB is entry of an almost diamond sub-CFG, with a
+                       // shared tail.
     };
----------------
nit: with a common tail that can be shared

================
Comment at: lib/CodeGen/IfConversion.cpp:499
@@ +498,3 @@
+  DebugLoc dl;
+  MachineBasicBlock::iterator BBIT = BBI.BB->getFirstTerminator();
+  if (BBIT != BBI.BB->end())
----------------
Split out the independent fix with a test case if possible

================
Comment at: lib/CodeGen/IfConversion.cpp:1006
@@ +1005,3 @@
+/// non predicable instructions are part of a shared tail.
+bool IfConverter::FeasibilityAnalysisSharedTail(
+    BBInfo &BBI, SmallVectorImpl<MachineOperand> &Pred) {
----------------
Document the parameters.

================
Comment at: lib/CodeGen/IfConversion.cpp:1006
@@ +1005,3 @@
+/// non predicable instructions are part of a shared tail.
+bool IfConverter::FeasibilityAnalysisSharedTail(
+    BBInfo &BBI, SmallVectorImpl<MachineOperand> &Pred) {
----------------
davidxl wrote:
> Document the parameters.
Why can't this function be folded into existing FeasbilityAnalysis with a new flag : hasCommonForkedTail  (which defaults to false) ?

================
Comment at: lib/CodeGen/IfConversion.cpp:1855
@@ +1854,3 @@
+
+  if (TrueBBI.BB->pred_size() > 1 || FalseBBI.BB->pred_size() > 1) {
+    BBI.IsAnalyzed = false;
----------------
Why is this check not done for the forked case?


https://reviews.llvm.org/D22551