[PATCH] Allow isDereferencablePointer to look through BitCast ConstantExprs

[Hal Finkel]

----- Original Message -----
> From: "Nick Lewycky" <nicholas at mxc.ca>
> To: hfinkel at anl.gov, "david majnemer" <david.majnemer at gmail.com>
> Cc: nicholas at mxc.ca, llvm-commits at cs.uiuc.edu
> Sent: Sunday, February 8, 2015 3:24:57 PM
> Subject: Re: [PATCH] Allow isDereferencablePointer to look through BitCast ConstantExprs
> 
> By the way, this isn't related to your patch per se but since you're
> working in the area I thought I should mention it. There's a small
> problem with isDereferenceablePointer and the dereferenceable
> attribute.
> 
>   define void @test(i8* dereferenceable %p) {
>     call void @free(i8* %p)
>     load i8* %p  ;; isDeferenceablePointer returns true.
>     ret void
>   }
> 
> The langref doesn't specify whether dereferenceable means that it's
> deref'able at the moment of function entry (what I intended) or
> deref'able for the lifetime of the function (what we're currently
> doing).

You're right. We should fix this. Too bad we don't have a source-level annotation for 'un-freeable memory'.

 -Hal

> We lower C++ reference types to dereferenceable, and I'm not
> sure that's correct with the second interpretation. I expect this
> code
> is legal C++:
> 
>   void t2(int &i) { free(&i); }
>   void t1() { int i = malloc(sizeof(int)); t2(*i); }
> 
> Just something to be aware of. If you try to write another patch to
> improve isDereferenceablePointer and code starts miscompiling, this
> mismatch may be why.
> 
> Nick
> 
> Michael Kuperstein wrote:
> 
> > Hi hfinkel,
> 
> > 
> 
> > isDereferencablePointer looks through BitCast instructions, but not
> > constant expressions.
> 
> >  This fixes PR22460.
> 
> > 
> 
> > I could have both sides of the if use the same code - but it would
> > have to be the one for the ConstantExpr side.
> 
> >  Is that better?
> 
> > 
> 
> > http://reviews.llvm.org/D7411
> 
> > 
> 
> > Files:
> 
> > 
> 
> >   lib/IR/Value.cpp
> 
> >   test/Transforms/LICM/constexpr.ll
> 
> > 
> 
> > Index: lib/IR/Value.cpp
> 
> >  ===================================================================
> 
> > 
> 
> >   - lib/IR/Value.cpp +++ lib/IR/Value.cpp @@ -495,17 +495,29 @@ //
> >   to a type of smaller size (or the same size), and the alignment
> >   // is at least as large as for the resulting pointer type, then
> >   // we can look through the bitcast.
> 
> > - if (DL)
> 
> > - if (const BitCastInst* BC = dyn_cast<BitCastInst>(V)) {
> 
> > - Type *STy = BC->getSrcTy()->getPointerElementType(),
> 
> > - *DTy = BC->getDestTy()->getPointerElementType();
> 
> > - if (STy->isSized()&&  DTy->isSized()&& +  if (DL) { +    Type
> > *STy = nullptr, *DTy = nullptr; +    Value *SrcVal = nullptr; + +
> >    if (const BitCastInst *BC = dyn_cast<BitCastInst>(V)) { +
> >      SrcVal = BC->getOperand(0); +      STy =
> > BC->getSrcTy()->getPointerElementType(); +      DTy =
> > BC->getDestTy()->getPointerElementType(); +    } else if (const
> > ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) { +      if
> > (CE->getOpcode() == Instruction::BitCast) { +        SrcVal =
> > CE->getOperand(0); +        STy =
> > SrcVal->getType()->getPointerElementType(); +        DTy =
> > CE->getType()->getPointerElementType(); +      } +    } + +    if
> > (STy&&  STy->isSized()&&  DTy->isSized()&&
> > (DL->getTypeStoreSize(STy)>= DL->getTypeStoreSize(DTy))&&
> > (DL->getABITypeAlignment(STy)>= DL->getABITypeAlignment(DTy)))
> 
> > - return isDereferenceablePointer(BC->getOperand(0), DL, Visited);
> 
> > - } +      return isDereferenceablePointer(SrcVal, DL, Visited); +
> >  }
> 
> > 
> 
> >   // Global variables which can't collapse to null are ok. if
> >   (const GlobalVariable *GV = dyn_cast<GlobalVariable>(V)) Index:
> >   test/Transforms/LICM/constexpr.ll
> >   ===================================================================
> 
> >   - test/Transforms/LICM/constexpr.ll +++
> >   test/Transforms/LICM/constexpr.ll @@ -0,0 +1,46 @@ +; RUN: opt<
> >    %s -S -basicaa -licm | FileCheck %s +; This fixes PR22460 +
> >   +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> >   +target triple = "x86_64-pc-windows-msvc" + + at in = internal
> >   unnamed_addr global i32* null, align 8 + at out = internal
> >   unnamed_addr global i32* null, align 8 + +; CHECK-LABEL: @bar +;
> >   CHECK: entry: +; CHECK: load i64* bitcast (i32** @in to i64*) +;
> >   CHECK: do.body: +; CHECK-NOT: load + +define i64 @bar(i32 %N) {
> >   +entry: +  br label %do.body + +do.body:
> >                                            ; preds = %l2, %entry +
> >    %i.0 = phi i32 [ 0, %entry ], [ %inc, %l2 ] +  %total = phi i64
> >   [ 0, %entry ], [ %next, %l2 ] +  %c = icmp eq i32 %N, 6 +  br i1
> >   %c, label %l1, label %do.body.l2_crit_edge +
> >   +do.body.l2_crit_edge:                             ; preds =
> >   %do.body +  %inval.pre = load i32** @in, align 8 +  br label %l2
> >   + +l1:                                               ; preds =
> >   %do.body +  %v1 = load i64* bitcast (i32** @in to i64*), align 8
> >   +  store i64 %v1, i64* bitcast (i32** @out to i64*), align 8 +
> >    %0 = inttoptr i64 %v1 to i32* +  br label %l2 + +l2:
> >                                                 ; preds =
> >   %do.body.l2_crit_edge, %l1 +  %inval = phi i32* [ %inval.pre,
> >   %do.body.l2_crit_edge ], [ %0, %l1 ] +  %int = ptrtoint i32*
> >   %inval to i64 +  %next = add i64 %total, %int +  %inc = add nsw
> >   i32 %i.0, 1 +  %cmp = icmp slt i32 %inc, %N +  br i1 %cmp, label
> >   %do.body, label %do.end + +do.end:
> >                                             ; preds = %l2 +  ret
> >   i64 %total +}
> 
> > 
> 
> >     EMAIL PREFERENCES
> >     http://reviews.llvm.org/settings/panel/emailpreferences/
> 
> > 
> 
> > _______________________________________________
> 
> >  llvm-commits mailing list
> 
> >  llvm-commits at cs.uiuc.edu
> 
> >  http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
> REPOSITORY
>   rL LLVM
> 
> http://reviews.llvm.org/D7411
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
> 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory