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

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:261
+static cl::opt<unsigned> HugeFuncThresholdInCGPP(
+    "Least BB number of huge function", cl::init(10000), cl::Hidden,
+    cl::desc("Consider building time when we encounter a huge function."));
----------------
Pls add cases for the patch with HugeFuncThresholdInCGPP be specified with small number.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:580
+
     for (BasicBlock &BB : llvm::make_early_inc_range(F)) {
+      if (FuncIterated && !FreshBBs.contains(&BB))
----------------
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.


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


================
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.
----------------
How can we remider devloper that FreshBB should be updated for their new code?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2151
   BasicBlock *StartBlock = CountZeros->getParent();
   BasicBlock *CallBlock = StartBlock->splitBasicBlock(CountZeros, "cond.false");
+  if (IsHugeFunc)
----------------
Ditto.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2428
+                                                ModifyDT &ModifiedDT) {
+  if (!BB->getTerminator())
+    return false;
----------------
Why need this change? Isn't it covered by line 2431?


================
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));
----------------
The instruction is cloned. Why the def of its operand need to be sinked?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7133
       LLVM_DEBUG(dbgs() << "Removing dead instruction: " << *I << "\n");
       I->eraseFromParent();
     }
----------------
Should we insert the BB to FreshBBs?


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:8217
+        if (IsHugeFunc)
+          break;
+        else
----------------
It should be buggy if the following instrcutions in the BB depends on the dominator tree.


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

https://reviews.llvm.org/D129352