Allow isDereferencablePointer to look through BitCast ConstantExprs

[mkuper]

Summary

isDereferencablePointer looks through BitCast instructions, but not constant expressions.
This fixes PR22460.

[hfinkel]

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?

Yes, I think that would be better. Do that, and LGTM.

[majnemer]

lib/IR/Value.cpp

502–511

I think you can reduce the code duplication by using Operator: http://llvm.org/docs/doxygen/html/classllvm_1_1Operator.html

Perhaps:

if (const auto *BC = dyn_cast<ConcreteOperator<Operator, Instruction::BitCast>>(V)) {
  SrcVal = BC->getOperand(0);
  STy = BC->getSrcTy()->getPointerElementType();
  DTy = BC->getDestTy()->getPointerElementType();
}

[mkuper]

Thanks Hal, David!
David, your suggestion almost works. What do you think about this?

[majnemer]

LGTM

[nicholas]

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). 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"
+
+@in = internal unnamed_addr global i32* null, align 8
+@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
[e-mail]
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

[hfinkel]

• Original Message -----

From: "Nick Lewycky" <[e-mail]>
To: [e-mail], "david majnemer" <[e-mail]>
Cc: [e-mail], [e-mail]
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" + +@in = internal unnamed_addr global i32* null, align 8 +@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

[e-mail]

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/

lib/IR/Value.cpp

static bool isDereferenceablePointer(const Value *V, const DataLayout *DL,
 
   // It's not always safe to follow a bitcast, for example:
   //   bitcast i8* (alloca i8) to i32*
   // would result in a 4-byte load from a 1-byte alloca. However,
   // if we're casting from a pointer from a type of larger size
   // 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))
     return !GV->hasExternalWeakLinkage();
 
   // byval arguments are okay. Arguments specifically marked as
   // dereferenceable are okay too.
   if (const Argument *A = dyn_cast<Argument>(V)) {

test/Transforms/LICM/constexpr.ll

+; 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"
+
+@in = internal unnamed_addr global i32* null, align 8
+@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
+}