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

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Currently, this change impacts all targets.  For those targets that don't implement the isImmediateInRangeForLoad API, do we expect this change to be an improvement in general?  If not, we might consider having isImmediateInRangeForLoad return an optional bool, so only those targets that implement isImmediateInRangeForLoad are impacted.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:399
@@ +398,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 represented
----------------
differend -> different

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:403
@@ +402,3 @@
+// in range.
+llvm::Optional<APInt> ConstantHoisting::calcOffsetDiff(APInt V1, APInt V2)
+{
----------------
Can this just be a static helper function?

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:405
@@ +404,3 @@
+{
+  llvm::Optional<APInt> Res;
+  unsigned BW = V1.getBitWidth() > V2.getBitWidth() ? V1.getBitWidth() : V2.getBitWidth();
----------------
Does this need to be initialized to 'None'?

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:411
@@ +410,3 @@
+  DEBUG(dbgs() << "Diffing " << V1 << " and " << V2 << "\n");
+  if (LimVal1 == ~0ULL || LimVal2 == ~0ULL) {
+      return Res;
----------------
No need for the extra brackets.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:462
@@ +461,3 @@
+      printOffsetRange(P1, P2);
+      if (P1 == P2 && P2WrappedAround) {
+        return BestOffset;
----------------
No need for the extra brackets.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:466
@@ +465,3 @@
+
+      if (P1 == P2) {
+        Gain = 0;
----------------
No need for the extra brackets.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:514
@@ +513,3 @@
+  unsigned NumUses = 0;
+  for (auto i = S; i != E; ++i) {
+    NumUses += i->Uses.size();
----------------
I'd prefer the original 'ConstCand' over just 'i'.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:704
@@ -567,3 +703,3 @@
       NumConstantsRebased++;
-      for (auto const &U : RCI.Uses)
+      for (auto const &U : RCI.Uses) {
         emitBaseConstants(Base, RCI.Offset, U);
----------------
No need for the extra brackets.


http://reviews.llvm.org/D21183