[llvm-commits] [llvm] r42270 - in /llvm/trunk: lib/Transforms/Scalar/LICM.cpp test/Transforms/LICM/2007-09-24-PromoteNullValue.ll

[Chris Lattner]

On Sep 24, 2007, at 1:02 PM, Devang Patel wrote:

> Author: dpatel
> Date: Mon Sep 24 15:02:42 2007
> New Revision: 42270
>
> URL: http://llvm.org/viewvc/llvm-project?rev=42270&view=rev
> Log:
>  Do not promote null values because it may be unsafe to do so.

Hi Devang,

Unfortunately, this approach is not the right way to go.

In the testcase you checked in, the fact that the store is to a null  
point is not the problem.  For example, you could change the testcase  
to store through a pointer argument, where a callee could pass in a  
null or other invalid pointer.

The issue is that the loop basically looks like this:


void foo() {
   for () {
     if (Cond) {
       *P += 1;
     }
   }
}

And ends up being transformed into:

void foo() {
   tmp = *P
   for () {
     if (Cond) {
       tmp += 1;
     }
   }
   *P = tmp;
}

In the former code, *P is only accessed if Cond is true, in the later  
case it is unconditionally accessed.  In your testcase, P is null,  
but that need not be the case: it could be any arbitrarily bad pointer.


Basically, it is only safe to do the memory promotion if the loop is  
guaranteed to access the memory location.  In this loop:

void foo() {
   for () {
      print(*P);
      if (Cond) {
       *P = 1;
     }
   }
}

it is safe to promote *P, because it has an unconditional use in the  
loop header.

Can you please continue hacking on this, solving the problem in its  
full generality?

-Chris


> Added:
>     llvm/trunk/test/Transforms/LICM/2007-09-24-PromoteNullValue.ll
> Modified:
>     llvm/trunk/lib/Transforms/Scalar/LICM.cpp
>
> Modified: llvm/trunk/lib/Transforms/Scalar/LICM.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/ 
> Scalar/LICM.cpp?rev=42270&r1=42269&r2=42270&view=diff
>
> ====================================================================== 
> ========
> --- llvm/trunk/lib/Transforms/Scalar/LICM.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/LICM.cpp Mon Sep 24 15:02:42 2007
> @@ -800,6 +800,10 @@
>            break;
>          }
>
> +      // Do not promote null values because it may be unsafe to do  
> so.
> +      if (isa<ConstantPointerNull>(V))
> +        PointerOk = false;
> +
>        if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(V)) {
>          // If GEP base is NULL then the calculated address used by  
> Store or
>          // Load instruction is invalid. Do not promote this value  
> because
>
> Added: llvm/trunk/test/Transforms/LICM/2007-09-24-PromoteNullValue.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ 
> LICM/2007-09-24-PromoteNullValue.ll?rev=42270&view=auto
>
> ====================================================================== 
> ========
> --- llvm/trunk/test/Transforms/LICM/2007-09-24-PromoteNullValue.ll  
> (added)
> +++ llvm/trunk/test/Transforms/LICM/2007-09-24-PromoteNullValue.ll  
> Mon Sep 24 15:02:42 2007
> @@ -0,0 +1,46 @@
> +; Do not promote null value because it may be unsafe to do so.
> +; RUN: llvm-as < %s | opt -licm | llvm-dis | not grep promoted
> +
> +define i32 @f(i32 %foo, i32 %bar, i32 %com) {
> +entry:
> +	%tmp2 = icmp eq i32 %foo, 0		; <i1> [#uses=1]
> +	br i1 %tmp2, label %cond_next, label %cond_true
> +
> +cond_true:		; preds = %entry
> +	br label %return
> +
> +cond_next:		; preds = %entry
> +	br label %bb
> +
> +bb:		; preds = %bb15, %cond_next
> +	switch i32 %bar, label %bb15 [
> +		 i32 1, label %bb6
> +	]
> +
> +bb6:		; preds = %bb
> +	%tmp8 = icmp eq i32 %com, 0		; <i1> [#uses=1]
> +	br i1 %tmp8, label %cond_next14, label %cond_true11
> +
> +cond_true11:		; preds = %bb6
> +	br label %return
> +
> +cond_next14:		; preds = %bb6
> +	store i8 0, i8* null
> +	br label %bb15
> +
> +bb15:		; preds = %cond_next14, %bb
> +	br label %bb
> +
> +return:		; preds = %cond_true11, %cond_true
> +	%storemerge = phi i32 [ 0, %cond_true ], [ undef, % 
> cond_true11 ]		; <i32> [#uses=1]
> +	ret i32 %storemerge
> +}
> +
> +define i32 @kdMain() {
> +entry:
> +	%tmp1 = call i32 @f( i32 0, i32 1, i32 1 )		; <i32> [#uses=0]
> +	call void @exit( i32 0 )
> +	unreachable
> +}
> +
> +declare void @exit(i32)
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits