[PATCH] D129352: [CodeGen] Limit building time for CodeGenPrepare

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:580
+
     for (BasicBlock &BB : llvm::make_early_inc_range(F)) {
+      if (FuncIterated && !FreshBBs.contains(&BB))
----------------
LuoYuanke wrote:
> As @nikic suggest, is it better to have worklist for the un-visited BB or changed BB? But it seems we may need to avoid double pushing BB.
FreshBBs is that worklist.
For huge function FreshBBs will collected all changed BB. 
(All un-visited BB will be visited at first iteration. Then set FuncIterated = true )


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1027
+                               bool IsHuge) {
+  auto *IOld = dyn_cast<Instruction>(Old);
+  if (IOld) {
----------------
LuoYuanke wrote:
> OldI (old instruction)?
let me refine it, thanks.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:1055
       // Merge DestBB into SinglePred/BB and delete it.
       MergeBlockIntoPredecessor(DestBB);
       // Note: BB(=SinglePred) will not be deleted on this path.
----------------
LuoYuanke wrote:
> How can we remider devloper that FreshBB should be updated for their new code?
Let me add requirement in FreshBB's comment.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2428
+                                                ModifyDT &ModifiedDT) {
+  if (!BB->getTerminator())
+    return false;
----------------
LuoYuanke wrote:
> Why need this change? Isn't it covered by line 2431?
The BB passed in can be no Terminator, so BB->getTerminator() may be nullptr.
dyn_cast can not handle nullptr.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7103
+      // So we put the operands defs' BBs into FreshBBs to do optmization.
+      for (unsigned I = 0; I < NI->getNumOperands(); ++I) {
+        auto *OpDef = dyn_cast<Instruction>(NI->getOperand(I));
----------------
LuoYuanke wrote:
> The instruction is cloned. Why the def of its operand need to be sinked?
When we copy an Instruction (to a new place) to do optimization, means we updated an instruction. This instruction's operand defs may have new opportunity to sink to the optimized new instruction.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7133
       LLVM_DEBUG(dbgs() << "Removing dead instruction: " << *I << "\n");
       I->eraseFromParent();
     }
----------------
LuoYuanke wrote:
> Should we insert the BB to FreshBBs?
Strictly speaking we should do.
But this instruction has been erased. All the opportunity about this "updated" instruction is disappear.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:8217
+        if (IsHugeFunc)
+          break;
+        else
----------------
LuoYuanke wrote:
> It should be buggy if the following instrcutions in the BB depends on the dominator tree.
The break will try re-iterate such BB.
Do you mean here the BB may be erased ?
I think the optimizeInst can not erase the BB.
If it split the BB, it will split it to the BB + new BB, the iteration on such BB still work.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129352/new/

https://reviews.llvm.org/D129352