[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: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."));
----------------
LuoYuanke wrote:
> Pls add cases for the patch with HugeFuncThresholdInCGPP be specified with small number.
Sure


================
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:
> xiangzhangllvm wrote:
> > 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.
> Here we just copy the instruction. We know it is sinked when it is erased in line 7135.
Let me try give an example soon.


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:8217
+        if (IsHugeFunc)
+          break;
+        else
----------------
LuoYuanke wrote:
> xiangzhangllvm wrote:
> > 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.
> I mean the dominator tree has changed but it is not updated yet. We can't invoke `DT.dominates(...)` before we updating the dominator tree.
Make sense, let me reset the DT here, thanks!


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

https://reviews.llvm.org/D129352