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

[Juergen Ributzka via llvm-commits]

ributzka added a comment.

Looks like a nice improvement to ConstantHoisting, but I am a little worried about the limited scope and implementation for load optimization only.

Constants are not only used by load/store instructions, so using "isImmediateInRangeForLoad" is very misleading. Also it might negatively impact decisions we used to make for other instructions.

Did you run the test suite to measure the performance and compile time impact for X86 and ARM/AArch64?


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:405
@@ -404,1 +404,3 @@
 
+  bool isImmediateInRangeForLoad(const APInt &Imm) const;
+
----------------
Please add a comment describing the new TTI method.

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:168
@@ +167,3 @@
+
+  bool CandidatesHaveUses(ConstCandVecType::iterator S, ConstCandVecType::iterator E);
+  llvm::Optional<APInt> calcOffsetDiff(APInt V1, APInt V2);
----------------
CandidatesHaveUses -> candidatesHaveUses 

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:169
@@ +168,3 @@
+  bool CandidatesHaveUses(ConstCandVecType::iterator S, ConstCandVecType::iterator E);
+  llvm::Optional<APInt> calcOffsetDiff(APInt V1, APInt V2);
+  ConstCandVecType::iterator maximizeConstantsInRange(ConstCandVecType::iterator S,
----------------
calcOffsetDiff -> calculateOffsetDiff

================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:460
@@ +459,3 @@
+    Diff = calcOffsetDiff(P2->ConstInt->getValue(), P1->ConstInt->getValue());
+    while (Diff && TTI->isImmediateInRangeForLoad(Diff.getValue())) {
+      printOffsetRange(P1, P2);
----------------
What about the case when the constant is not used by a load?

================
Comment at: test/Transforms/ConstantHoisting/X86/phi.ll:23
@@ -22,2 +22,3 @@
 ; CHECK: if.end:
-; CHECK: %2 = inttoptr i64 %const to i8*
+; OLDHECK: %2 = inttoptr i64 %const to i8*
+; CHECK: [[CONSTMAT:%const_mat[0-9]*]] = add i64 %const, 1
----------------
OLDHECK???


http://reviews.llvm.org/D21183