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

[Marina Yatsina via Phabricator via llvm-commits]

myatsina added inline comments.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:146
+  struct MBBInfo {
+    // Keeps clearance and domain information for all registers. Not that this
+    // is different from the usual definition notion of liveness. The CPU
----------------
Not --> Note ?


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:413
+    auto fi = MBBInfos.find(*pi);
+    assert(fi != MBBInfos.end());
+    LiveReg *Incoming = fi->second.OutRegs;
----------------
Can you add an assertion message please?


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:415
+    LiveReg *Incoming = fi->second.OutRegs;
+    if (Incoming == nullptr) {
       continue;
----------------
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.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:456
   // Save live registers at end of MBB - used by enterBasicBlock().
   // Also use LiveOuts as a visited set to detect back-edges.
+  MBBInfos[MBB].OutRegs = LiveRegs;
----------------
Some of the comments in this function need to be updated (we no longer have LiveOuts, we always change the defs to be ralive to the end of the block etc)


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:571
+  // and by then we'll have better information, so we can avoid this work now.
+  if (BlockDone) {
+    unsigned Pref = TII->getUndefRegClearance(*MI, OpNum, TRI);
----------------
For readability purpose - how about changing "BlockDone" to "breakDependency" and add your comment regarding done blocks before the call to processDefs?

processDefs() will look like this:
if (breakDependency) {
 // calc Pref
...
}

and processBasicBlock will look like this:

// If this block is not done, it makes little sense ...
bool breakDependency = isBlockDone(Done)
processDefs(MI, breakDependency, ...)



================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:792
+void ExeDepsFix::processBasicBlock(MachineBasicBlock *MBB, bool PrimaryPass,
+                                   bool Done) {
+  enterBasicBlock(MBB);
----------------
Do you need "Done" here? I don't see you using it.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:802
+  }
+  processUndefReads(MBB);
+  leaveBasicBlock(MBB);
----------------
Is there a point going over it when we're not isBlockDone?
processDefs pushes instructions into the undef read only when Done =true.
processUndefReads does break if the undef reads are empty, but perhaps for readability purpose it's worth writing this explicitly.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:819
+      }
+      if (Done) {
+        MBBInfos[Succ].IncomingCompleted++;
----------------
Why not check isBasicBlockDone on MBB?


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:822
+      }
+      if (isBlockDone(Succ)) {
+        processBasicBlock(Succ, false, true);
----------------
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. 



================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:868
+  for (auto &MBB : mf) {
+    MBBInfo InitialInfo{nullptr, false, 0, 0, 0};
+    MBBInfos.insert(std::make_pair(&MBB, InitialInfo));
----------------
Better use a constructor with default initialization like DomainValue does.


================
Comment at: lib/CodeGen/ExecutionDepsFix.cpp:878
+    MBBInfos[MBB].PrimaryCompleted = true;
+    MBBInfos[MBB].PrimaryIncoming = MBBInfos[MBB].IncomingProcessed;
+    bool PrimaryDone = isBlockDone(MBB);
----------------
I would add a comment that IncomingProcessed and IncomingCompleted of this block were already updated during the processing of predecessor blocks.


https://reviews.llvm.org/D28759