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

[David Blaikie via llvm-commits]

On Thu, Jan 21, 2016 at 6:14 PM, Eduard Burtescu via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> 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.
>

I think it'd be best to preserve the old behavior. Feel free to add a //
FIXME: Is this really the right thing, shouldn't we use the blah blah blah
or the like.


>
> ================
> 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.
>

So was this check unnecessary all along? Or are callers doing an equivalent
check now?


>
> 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.
>

If it's obviously i8* in the code, that's OK (could you tell me where to
look - even if it's a few lines nearby, that'd be helpful), might be worth
a comment or assertion.


>
>
> http://reviews.llvm.org/D16425
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160121/a33852b6/attachment.html>