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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Looks generally good - I just don't understand how some of the changes are valid. Would be helpful to know.



================
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;
-    }
----------------
Is there a guarantee that it's never really a pointer type here?


================
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);
----------------
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)


================
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;
-
----------------
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)


https://reviews.llvm.org/D26595