Dereferencing null pointers

Hi,

I would like to understand better the meaning of constant null pointer in LLVM IR.

Can the optimizer assume that dereferencing a null pointer is always unreachable? Or is it only the case for address space 0? Is it ok to have null pointer be a valid pointer for an address space other than 0?

In InstCombine pass in InstCombiner::visitLoadInst(LoadInst &LI) I see that we replace load of null pointer by unreachable only for address space 0. But there is also code doing the following transformation for all the address spaces:

// load (select (cond, null, P)) → load P

if(isa(SI->getOperand(1)) &&

LI.getPointerAddressSpace() == 0) {

LI.setOperand(0, SI->getOperand(2));

return &LI;

}

Is this a bug? Would the correct behavior be to check that the pointers’ address space is 0?

Cheers,

Thomas

From: "Thomas F Raoux" <thomas.f.raoux@intel.com>
To: llvmdev@cs.uiuc.edu
Sent: Thursday, December 11, 2014 1:03:39 PM
Subject: [LLVMdev] Dereferencing null pointers

Hi,

I would like to understand better the meaning of constant null
pointer in LLVM IR.

Can the optimizer assume that dereferencing a null pointer is always
unreachable? Or is it only the case for address space 0? Is it ok to
have null pointer be a valid pointer for an address space other than
0?

Correct, it is valid to have a null pointer dereference in address spaces other than 0.

In InstCombine pass in InstCombiner::visitLoadInst(LoadInst &LI) I
see that we replace load of null pointer by unreachable only for
address space 0. But there is also code doing the following
transformation for all the address spaces:

// load (select (cond, null, P)) -> load P

if(isa<ConstantPointerNull>(SI->getOperand(1)) &&

LI.getPointerAddressSpace() == 0) {

LI.setOperand(0, SI->getOperand(2));

return &LI;

}

Is this a bug? Would the correct behavior be to check that the
pointers’ address space is 0?

But it does, no? it has && LI.getPointerAddressSpace() == 0

-Hal

Sorry for the confusion, I pasted the code I fixed locally...

Here is the code at top of the trunk:

      // load (select (cond, null, P)) -> load P
      if (Constant *C = dyn_cast<Constant>(SI->getOperand(1)))
        if (C->isNullValue()) {
          LI.setOperand(0, SI->getOperand(2));
          return &LI;
        }

So it is a bug?

Yup, that seems like a bug. I confirmed that opt -O3 transforms

define i32 @foo(i1 %c, i32 addrspace(2) *%a) {
entry:
  %ptr = select i1 %c, i32 addrspace(2)* null, i32 addrspace(2)* %a
  %v = load i32 addrspace(2)* %ptr
  ret i32 %v
}

to

define i32 @foo(i1 %c, i32 addrspace(2)* nocapture readonly %a) #0 {
entry:
  %v = load i32 addrspace(2)* %a
  ret i32 %v
}

attributes #0 = { nounwind readonly }

Is there a principled way to prevent such bugs? Perhaps
ConstantPointerNull should imply addrspace(0), to get to the other
addrspaces, you need an addrspacecast?

-- Sanjoy

From: "Sanjoy Das" <sanjoy@playingwithpointers.com>
To: "Thomas F Raoux" <thomas.f.raoux@intel.com>
Cc: "Hal Finkel" <hfinkel@anl.gov>, llvmdev@cs.uiuc.edu
Sent: Thursday, December 11, 2014 1:59:10 PM
Subject: Re: [LLVMdev] Dereferencing null pointers

Yup, that seems like a bug. I confirmed that opt -O3 transforms

Agreed.

define i32 @foo(i1 %c, i32 addrspace(2) *%a) {
entry:
  %ptr = select i1 %c, i32 addrspace(2)* null, i32 addrspace(2)* %a
  %v = load i32 addrspace(2)* %ptr
  ret i32 %v
}

to

define i32 @foo(i1 %c, i32 addrspace(2)* nocapture readonly %a) #0 {
entry:
  %v = load i32 addrspace(2)* %a
  ret i32 %v
}

attributes #0 = { nounwind readonly }

Is there a principled way to prevent such bugs? Perhaps
ConstantPointerNull should imply addrspace(0), to get to the other
addrspaces, you need an addrspacecast?

This is an interesting idea, but I'm not sure this is worth it. This is also used to optimize null pointer checks, and those can occur in any address space. There are several places where we need to check the address space on load/store optimizations (and we do), and we just need to do that here as well.

Can one of you submit a patch to llvm-commits?

-Hal

I'll submit a patch by the end of the week.

-Thomas