[PATCH] D21183: Better selection of common base address in constant hoisting

[James Molloy via llvm-commits]

jmolloy added inline comments.

================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:413
@@ +412,3 @@
+
+  /// \brief Return the expected cost for the given integer when optimising
+  /// for size. This is different than the other integer immediate cost
----------------
Having this hook be context-free will hinder targets implementing it usefully. It should take the same arguments as getIntImmCost(), so that targets can give a contextual answer if they wish.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:288
@@ -287,1 +287,3 @@
 
+// This helper function is necessary to deal with values that have differend
+// bit widths (APInt Operator- does not like that). If the value cannot be
----------------
"different"

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:341
@@ +340,3 @@
+    DEBUG(dbgs() << "= Constant: " << ConstCand->ConstInt->getValue() << "\n");
+    for (auto User = ConstCand->Uses.begin(); User != ConstCand->Uses.end(); ++User) {
+      auto Value = ConstCand->ConstInt->getValue();
----------------
You can use a range-for loop here.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:342
@@ +341,3 @@
+    for (auto User = ConstCand->Uses.begin(); User != ConstCand->Uses.end(); ++User) {
+      auto Value = ConstCand->ConstInt->getValue();
+      Cost += TTI->getIntImmCost(User->Inst->getOpcode(), User->OpndIdx,
----------------
This should be hoisted out of the inner loop as it is it invariant.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:347
@@ +346,3 @@
+      for (auto C2 = S; C2 != E; ++C2) {
+        llvm::Optional<APInt> Diff = calculateOffsetDiff(
+                                      C2->ConstInt->getValue(),
----------------
The formatting looks off here - have you used clang-format?


http://reviews.llvm.org/D21183