[PATCH] D26595: IR: Change PointerType to derive from Type rather than SequentialType.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked an inline comment as done.
pcc added inline comments.


================
Comment at: llvm/lib/IR/ConstantFold.cpp:2208-2212
-    if (isa<PointerType>(STy)) {
-      // We don't know if it's in range or not.
-      Unknown = true;
-      continue;
-    }
----------------
dblaikie wrote:
> Is there a guarantee that it's never really a pointer type here?
The purpose of this loop is to look through a notional GEP's element types. Earlier we initialized `Ty` to `PointeeTy` which, at the only call site [0], is the notional GEP's source element type. Because the source element type cannot be a pointer, and the same goes for later element types, having a pointer type here is impossible and this was dead code before.

[0] http://llvm-cs.pcc.me.uk/lib/IR/Constants.cpp#1906


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:178
   // For homogenous sequential types, check for padding within members.
   if (SequentialType *seqTy = dyn_cast<SequentialType>(type))
+    return isDenselyPacked(seqTy->getElementType(), DL);
----------------
dblaikie wrote:
> This looks like it changes the behavior of the code.
> 
> Previously the code returned here if type was a PointerType - now it doesn't return, but continues into the rest of the function.
> 
> Is this just because type was never really a PointerType? (perhaps you could add an assert to that effect)
Now that PointerTypes are no longer CompositeTypes, the return happens on line 175 above.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3215-3219
   if (SequentialType *SeqTy = dyn_cast<SequentialType>(Ty)) {
-    // We can't partition pointers...
-    if (SeqTy->isPointerTy())
-      return nullptr;
-
----------------
dblaikie wrote:
> This also appears to change the behavior - if Ty is a PointerType this code previously returned, now it skips this block and continues. Is this correct? Because Ty is never a PointerType here (add an assert?), or because the following code doesn't do anything if it is? (maybe still worth an early return defensively - oh, I see, after this block it returns if non-struct, so that's pretty obvious & maybe doesn't need anything extra)
Yes, I think the existing code is sufficient.


https://reviews.llvm.org/D26595