[PATCH] D28962: Add BFI in constanthoisting pass and do the hoisting selectively

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:678
+      Instruction *Base =
+          new BitCastInst(ConstInfo.BaseConstant, Ty, "const", IP);
+      DEBUG(dbgs() << "Hoist constant (" << *ConstInfo.BaseConstant
----------------
wmi wrote:
> davidxl wrote:
> > davidxl wrote:
> > > There are three cases:
> > > 1) new insertion point introduced
> > > 2) the BB originally has a base constant use
> > > 3) the BB originally has a constant use but need rebase.
> > > 
> > > For case 1) and 3), new insertion is needed, but for 2) it can be skipped.
> > Correction: 2) can only be skipped when there are no other uses dominated by it -- so it is probably better to not to skip and rewrite the original use
> I thought it was not going to change the generated code if we do a rebase for a base constant use. That is why for the code simplicity, I choose to insert materialization and rebase code anyway.  
> 
> It will change 
> 
> BB:
> %inc = add i64 %5, 214748364701
> 
> to
> 
> BB:
> %const = bitcase i64 214748364701 to i64
>  ...
> %inc = add i64 %5, %const
> 
> Because 214748364701 is a very large number (The precondition it is treated as constant hoisting candidate) and it cannot be encoded in add instruction, we need the materialization code %const = ...  anyway in the end. 
Ok makes sense.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:694
+          // a use is the Base dominating the use.
+          if (!BFI || DT->dominates(Base->getParent(), OrigMatInsertBB)) {
+            emitBaseConstants(Base, RCI.Offset, U);
----------------
Should !BFI check removed? It just improves compile time a little for the non-default path. To save compile time for more generally, do 'if (IPSet->size() == 1 || DT->dominates(...)) instead?

Also is it better to keep track of the base constants rewritten and assert at the end that all uses are rewritten.




Repository:
  rL LLVM

https://reviews.llvm.org/D28962