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

David Li via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 2 10:25:00 PDT 2016


davidxl added inline comments.

================
Comment at: lib/CodeGen/IfConversion.cpp:703
@@ +702,3 @@
+
+  MachineBasicBlock *TT = TrueBBI.TrueBB;
+  MachineBasicBlock *TF = TrueBBI.FalseBB;
----------------
iteratee wrote:
> 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.
Ok -- the naming convention makes sense.

================
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.
----------------
iteratee wrote:
> 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.
My question in this comment is that countDuplicatiedInstruction does not document exit state of TIE and FIE, so  add assertions to make sure TIE and FIE point to what you expect to see (pointing to identical instructions before and not identical after ?)

================
Comment at: lib/CodeGen/IfConversion.cpp:1116
@@ +1115,3 @@
+    BBInfo TrueBBICalc, FalseBBICalc;
+    if (CanRevCond && ValidForkedDiamond(TrueBBI, FalseBBI, Dups, Dups2,
+                                         TrueBBICalc, FalseBBICalc) &&
----------------
iteratee wrote:
> 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.
I still think refactoring is better -- the main reason of doing the refactoring is to avoid code duplication which is better longer term. For instance, no need to worry about fixing bugs in multiple different places.

Another point is that Diamond and ForkedDiamond patterns are exclusive, so there is no need to do forked diamond check after diamond check returns true. In other words, the code can be further simplified to:

if (CanRevCond) {
     if (!DiamondFinder(...::ValidDiamond)) {
         DiamondFilnder(... ::ValidForkedDiamond);
}

Also it seems to me you don't need to introduce TrueBBICalc and FalseBBICalc. How about in ValidForkedDiamond
1) saving the initial value of TrueBBI and FalseBBI
2) re-scan region of BB and update TrueBBI and FalseBBI
3) if ValidForkedDiamond fails, restore TrueBBI etc value before returning.


================
Comment at: lib/CodeGen/IfConversion.cpp:791
@@ +790,3 @@
+    if (BBIRecalc.ClobbersPred)
+      return false;
+    if (TII->isPredicated(*BIB))
----------------
Should this be done for ValidDiamond too -- only check the non-shared portion?

Also I don't think it is ideal to have code duplication like this.   Looks like you should re-use scanInstructions or part of it (by making it accepting BIB and BIE) ?


https://reviews.llvm.org/D22551





More information about the llvm-commits mailing list