[PATCH] [SLSR] handle candidate form &B[i * S]

Jingyue Wu jingyue at google.com
Wed Mar 11 14:38:25 PDT 2015


================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:363
@@ +362,3 @@
+    // sign-extended to the pointer size.
+    for (unsigned Stripped = 0; Stripped < 2; ++Stripped) {
+      // At least, ArrayIdx = ArrayIdx *s 1.
----------------
meheff wrote:
> I see that this loop enables you to avoid code duplication for the two cases (sext-wrapped and no sext), but it requires a bit of mental effort to see what it is doing.  I think this would be clearer if you just broke out the code into a separate function and avoided the loop altogether.  Then the code looks like:
> 
> foobarfunction(Base, ArrayIdx, ElementSize, GEP)
> Value *SextIdx;
> if (!match(ArrayIdx, m_SExt(m_Value(SextIdx)))
>   foobarfunction(Base, SextIdx, ElementSize, GEP);
> 
> In this case it is much more obvious that you're considering the two cases (sext and no sext).  There is one other location in the file which uses this similar style loop construct and might be clearer expressed as a separate function call.
> 
> 
> 
> 
> 
> 
Both places fixed. PTAL. 

================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:461
@@ +460,3 @@
+      Type *IntPtrTy = DL->getIntPtrType(C.Ins->getType());
+      // We prefer bumping with GEP because GEPs keep more pointer information.
+      if (BumpWithUglyGEP) {
----------------
hfinkel wrote:
> Either remove this comment, or say that we use i8* GEPs, instead of inttoptr/ptrtoint because the latter interferes with pointer-aliasing analysis.
> 
Missed this comment. Fixed in this patch.

http://reviews.llvm.org/D7459

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list