[PATCH] D46191: [LV] Preserve inbounds on created GEPs

[Daniel Neilson via Phabricator via llvm-commits]

dneilson added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3128
 
+    bool InBounds = false;
+    if (auto *gep = dyn_cast<GetElementPtrInst>(
----------------
hsaito wrote:
> Would you try using getLoadStorePointerOperand() from Instructions.h (see r327173) and hoist this code above "if (SI)", to cover both LI and SI at the same time?
> 
Didn't know that exists. Definitely will use that; makes this cleaner. Thanks for pointing it out.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3160
+          PartPtr = cast<GetElementPtrInst>(
+              Builder.CreateGEP(nullptr, PartPtr, Builder.getInt32(1 - VF)));
+          PartPtr->setIsInBounds(InBounds);
----------------
hsaito wrote:
> I wonder why this isn't done using a single GEP...... If we use a single GEP, we can clean up a little more.
Good question.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3197
       // Calculate the pointer for the specific unroll-part.
-      Value *PartPtr =
-          Builder.CreateGEP(nullptr, Ptr, Builder.getInt32(Part * VF));
+      auto *PartPtr = cast<GetElementPtrInst>(
+          Builder.CreateGEP(nullptr, Ptr, Builder.getInt32(Part * VF)));
----------------
hsaito wrote:
> Address computation part of LI and SI code paths should be identical. We probably should clean this up, but I suppose I shouldn't be asking you to do that as part of this patch. Just a note.
The clear copy/paste really struck me as well. I'll do an NFC to clean this up...


Repository:
  rL LLVM

https://reviews.llvm.org/D46191