[PATCH] D16417: [opaque pointer types] [NFC] {Load, Store}Inst: get loaded/stored type from the instruction.

[Eduard Burtescu via llvm-commits]

eddyb added inline comments.

================
Comment at: lib/Target/PowerPC/PPCLoopPreIncPrep.cpp:209
@@ -205,4 +208,3 @@
       // There are no update forms for Altivec vector load/stores.
-      if (ST && ST->hasAltivec() &&
-          PtrValue->getType()->getPointerElementType()->isVectorTy())
+      if (ST && ST->hasAltivec() && ValTy && ValTy->isVectorTy())
         continue;
----------------
mjacob wrote:
> I think you changed semantics here. What if the instruction is a prefetch intrinsic with a pointer to a vector as the first operand?
Prefetch is monomorphic, it only takes one pointer, AFAICT.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2437
@@ -2436,2 +2436,3 @@
       }
-      if (!isSimpleEnoughPointerToCommit(Ptr)) {
+      Type *ValTy = SI->getValueOperand()->getType();
+      if (!isSimpleEnoughPointerToCommit(Ptr, ValTy)) {
----------------
mjacob wrote:
> Do we need this variable?
No, although, I might put `SI->getValueOperand()` in a variable just to keep the line length in check.

================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:268-270
@@ -270,3 +267,5 @@
 // vector or a pointer to a vector.
-Scatterer Scalarizer::scatter(Instruction *Point, Value *V) {
+Scatterer Scalarizer::scatter(Instruction *Point, Value *V, Type *VecTy) {
+  if (!VecTy)
+    VecTy = V->getType();
   if (Argument *VArg = dyn_cast<Argument>(V)) {
----------------
mjacob wrote:
> Can you document (and possibly add an assertion for) the requirements on VecTy?
> 
> If I understand correctly, there are two possibilities:
> 1) `V` is a vector and `VecTy` is nullptr.
> 2) `V` is a pointer to a vector and `VecTy` is the pointee type.
> 
> At least we should add an assertion that `V` is a vector if `VecTy` is nullptr.
I agree with that - I also need to extract the element type and use it, which for some reason I didn't do back when I first made this change.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1991
@@ -1990,3 +1990,3 @@
   // Make sure that the pointer does not point to structs.
-  if (Ptr->getType()->getPointerElementType()->isAggregateType())
+  if (DataType && DataType->isAggregateType())
     return 0;
----------------
mjacob wrote:
> eddyb wrote:
> > eddyb wrote:
> > > dblaikie wrote:
> > > > Do we need to support a null type parameter here?
> > > This might be an artifact of rebasing, i.e. it was needed but now it's not.
> > > I'll try testing without the `nullptr` default and this check.
> > Ah, that call site doesn't show up in the commit because it hasn't changed.
> > See the `collectLoopUniforms` method.
> What if `isConsecutivePtr` is called from `collectLoopUniforms` with a pointer to a struct?
Honestly, I'm not sure. I don't think that it affects the resulting decisions, but I can try to look deeper into it.


http://reviews.llvm.org/D16417