[PATCH] D52243: [ConstHoist] Do not rebase single (or few) dependent constant

[Z. Zheng via Phabricator via llvm-commits]

zzheng marked 4 inline comments as done.
zzheng added inline comments.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:873
+      if (ToBeRebased.size() < MinNumOfDependentToRebase) {
+        Base->eraseFromParent();
+        NotRebasedNum += ToBeRebased.size();
----------------
dmgreen wrote:
> zzheng wrote:
> > dmgreen wrote:
> > > Can you change this around so we don't speculatively make the Base? That seems to be the way of doing things in LLVM.
> > Here Base is not the base constant, but an instance (insertion point) of it. A base constant can have multiple insertion points. The existing algorithm first finds the insertion points, then looks for rebased uses dominated by each insertion point.
> > 
> > If I understand your comments correctly, it's desirable to not to have Base at the first place so no need to erase it later. But we need the number of rebased uses to determine if we'll rewrite such uses. To get this number we need Base.
> > 
> > I can remove the statement:
> > ```
> > Base->eraseFromParent();
> > ```
> > Since Base has no use, it'll be trivially removed by DCE.
> (I think) because we are inserting Base at IP, Base->getParent will just be IP->getParent, and if we use that we can move the code to create Base (and the debug message after) to after this check.
> 
> Unless findMatInsertPt is doing something I don't expect and requiring that Base has been created? Make sure that works, and all the tests are passing.
Ah, didn't see this. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D52243