[PATCH] D51396: [Constant Hoisting] Hoisting Constant GEP Expressions

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

A few more comments with minor tweaks.  Otherwise looks good.



================
Comment at: include/llvm/Transforms/Scalar/ConstantHoisting.h:182
   void collectConstantCandidates(Function &Fn);
+  void computeGEPOffset(ConstantExpr *ConstExpr, Type *Ty, APInt &Offset);
   void findAndMakeBaseConstant(ConstCandVecType::iterator S,
----------------
Dead declaration.


================
Comment at: include/llvm/Transforms/Scalar/ConstantHoisting.h:192
                          const consthoist::ConstantUser &ConstUser);
-  bool emitBaseConstants();
+  bool emitBaseConstants(GlobalVariable *BaseGV = nullptr);
   void deleteDeadCastInst() const;
----------------
Don't use default arguments for functions like this... it's more confusing than helpful.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:732
+      // Constant being rebased is a ConstantExpr.
+      PointerType *Int8PtrTy = Type::getInt8PtrTy(*Ctx);
+      Base = new BitCastInst(Base, Int8PtrTy, "base_bitcast", InsertionPt);
----------------
You probably should specify the address-space here, to the same address-space as Ty.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:788
+    }
     Instruction *ConstExprInst = ConstExpr->getAsInstruction();
     ConstExprInst->setOperand(0, Mat);
----------------
Could you add an assertion here `assert(ConstExpr->isCast())`, so it's clear this matches the code in collectConstantCandidates?


Repository:
  rL LLVM

https://reviews.llvm.org/D51396