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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/ConstantHoisting.h:81
   ConstantInt *ConstInt;
+  ConstantExpr *ConstExpr;
   unsigned CumulativeCost = 0;
----------------
Could use a comment explaining what ConstExpr and ConstInt represent.


================
Comment at: include/llvm/Transforms/Scalar/ConstantHoisting.h:110
+  ConstantInt *BaseInt;
+  ConstantExpr *BaseExpr;
+  Type *BaseTy;
----------------
Could use a comment explaining what BaseExpr and BaseInt represent.


================
Comment at: include/llvm/Transforms/Scalar/ConstantHoisting.h:111
+  ConstantExpr *BaseExpr;
+  Type *BaseTy;
   RebasedConstantListType RebasedConstants;
----------------
I'm sort of confused by the usage of BaseTy; why does the type of the base matter?  It just gets bitcast away later, anyway.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:412
+  if (Cost > TTI->getIntImmCost(Instruction::Add, 1, Offset, PtrIntTy) ||
+      SizeCost > TTI->getIntImmCodeSizeCost(
+                     Instruction::Add, 1, Offset, PtrIntTy)) {
----------------
getIntImmCodeSizeCost returns either zero or one on ARM... so it's always less than SizeCost?


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:745
+                                 "const_mat", InsertionPt);
+      Mat = new IntToPtrInst(Mat, Ty, "mat_inttoptr", InsertionPt);
+    } else
----------------
Please use bitcast+GEP+bitcast, instead of ptrtoint/inttoptr.


Repository:
  rL LLVM

https://reviews.llvm.org/D51396