[PATCH] D16425: [opaque pointer types] [NFC] isDereferenceable{, AndAligned}Pointer: take the accessed size (and alignment) as arguments.

[Eduard Burtescu via llvm-commits]

eddyb added inline comments.

================
Comment at: include/llvm/Analysis/ValueTracking.h:244
@@ -244,1 +243,3 @@
+  bool isDereferenceablePointer(const Value *V, uint64_t Size,
+                                const DataLayout &DL,
                                 const Instruction *CtxI = nullptr,
----------------
dblaikie wrote:
> Do we still need to pass the DataLyaout to these functions if they have the size & alignment?
Yes, it's used all over the place in ValueTracking (to get the sizes of other types found during the process).

================
Comment at: lib/Analysis/ValueTracking.cpp:3260
@@ +3259,3 @@
+    unsigned BaseAlign = DL.getABITypeAlignment(BaseType);
+    if (BaseAlign < Align)
+      BaseAlign = Align;
----------------
dblaikie wrote:
> Where did this case come from?
Previously, if `Align` was 0, then `BaseAlign` would take over.
Instead, I'm taking the maximum alignment required by either, but I'm not 100% sure it does the exact same thing, only that it passes the tests.

================
Comment at: lib/Analysis/ValueTracking.cpp:3308
@@ -3307,3 +3302,1 @@
 
-  if (Ty->isSized()) {
-    APInt Offset(DL.getTypeStoreSizeInBits(VTy), 0);
----------------
dblaikie wrote:
> Where'd this check go?
Based on the fact that you can't get the `Size` necessary to call this function if you have an unsized type.

But I can add the check back, as `if (Size > 0)`, if you want.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1779
@@ -1778,3 +1778,3 @@
     // isDereferenceablePointer -> deref attribute
-    if (isDereferenceablePointer(DerivedPtr, DL)) {
+    if (isDereferenceablePointer(DerivedPtr, 1, DL)) {
       if (Argument *A = dyn_cast<Argument>(DerivedPtr)) {
----------------
dblaikie wrote:
> Why is it only '1', rather than 'Bytes' used below?
I recall this being `i8*`, but if that is not the case, I'll use `Bytes` instead.


http://reviews.llvm.org/D16425