[PATCH] D28759: [ExecutionDepsFix] Improve clearance calculation for loops

[Keno Fischer via Phabricator via llvm-commits]

loladiro added inline comments.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:415
+    LiveReg *Incoming = fi->second.OutRegs;
+    if (Incoming == nullptr) {
       continue;
----------------
myatsina wrote:
> Is OutRegs null when it's a back edge from a BB we haven't seen yet? Are there other cases where it can be null?
> Please add a comment explaining the cases.
Yes, that's correct. If it's null it's a back edge. Will add a comment to this extent.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:819
+      }
+      if (Done) {
+        MBBInfos[Succ].IncomingCompleted++;
----------------
myatsina wrote:
> Why not check isBasicBlockDone on MBB?
Just to avoid doing the calculation twice. Probably premature optimization. Will simplify.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:822
+      }
+      if (isBlockDone(Succ)) {
+        processBasicBlock(Succ, false, true);
----------------
myatsina wrote:
> At first glance, it wasn't clear to me why you need it here and why can't you just do one "primary" pass and then then process the basic blocks that are still not done.
> If I understood correctly, you're doing it here for the optimization you've talked about (Making sure the order is: PH A B C A' B' C' D). Am I right?
> I would add a comment here elaborating that.
> I would even consider adding as a comment somewhere with the loop example from the description of this patch and the "optimized" order the algorithm visits the nodes. I think it is a great example and it will make the traverse order here much clearer. 
> 
> If I understood correctly, you're doing it here for the optimization you've talked about (Making sure the order is: PH A B C A' B' C' D). Am I right?

Yes, that's correct. It's discussed below when going over the blocks that are not done. I'll add a small comment here. I will add the loop example from the commit message below and add a small comment here pointing people to the discussion below.


https://reviews.llvm.org/D28759