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

[Hideki Saito via Phabricator via llvm-commits]

hsaito added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3128
 
+    bool InBounds = false;
+    if (auto *gep = dyn_cast<GetElementPtrInst>(
----------------
dneilson wrote:
> 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.
Welcome. I wrote it. There used to be six getPointerOperand() file static functions. I didn't want to write a seventh one. Now it's readily available, it's time to advertise.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3138
         Value *MaskPart = isMaskRequired ? Mask[Part] : nullptr;
         Value *VectorGep = getOrCreateVectorValue(Ptr, Part);
         NewSI = Builder.CreateMaskedScatter(StoredVal, VectorGep, Alignment,
----------------
While you are at this, please look into whether isInbounds() is supported for vector gep. Whether anyone uses that is a different question, but losing information unnecessarily is sad. Same for Load side.


================
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)));
----------------
dneilson wrote:
> 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...
appreciated.


Repository:
  rL LLVM

https://reviews.llvm.org/D46191