[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1532
+      return llvm::ConstantInt::get(ConvertType(DestTy),
+          CGF.getContext().getTargetNullPtrValue(E->getType()));
     assert(!DestTy->isBooleanType() && "bool should use PointerToBool");
----------------
rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > yaxunl wrote:
> > > > rjmccall wrote:
> > > > > yaxunl wrote:
> > > > > > rjmccall wrote:
> > > > > > > Why is this necessary?  ptrtoint on the recursively-emitted null pointer should do this automatically.
> > > > > > Since the target knows the value in the null pointers, it can fold a null pointer to integer literal directly.
> > > > > > 
> > > > > > The above code does that, e.g.
> > > > > > 
> > > > > > ```
> > > > > > void test_cast_null_pointer_to_sizet_calee(size_t arg_private,
> > > > > >                                            size_t arg_local,
> > > > > >                                            size_t arg_global,
> > > > > >                                            size_t arg_constant,
> > > > > >                                            size_t arg_generic);
> > > > > > 
> > > > > > // CHECK-LABEL: test_cast_null_pointer_to_sizet
> > > > > > // CHECK: call void @test_cast_null_pointer_to_sizet_calee(i64 -1, i64 -1, i64 0, i64 0, i64 0)
> > > > > > void test_cast_null_pointer_to_sizet(void) {
> > > > > >   test_cast_null_pointer_to_sizet_calee((size_t)((private char*)0),
> > > > > >                                         (size_t)((local char*)0),
> > > > > >                                         (size_t)((global char*)0),
> > > > > >                                         (size_t)((constant char*)0),
> > > > > >                                         (size_t)((generic char*)0));
> > > > > > }
> > > > > > 
> > > > > > ```
> > > > > > 
> > > > > > Without the above code, we only get ptrtoint instructions.
> > > > > Oh, does the constant folder not know how to fold ptrtoint(inttoptr(X)) -> X?  I guess that's probably LLVM's nominal target-independence rearing its head.
> > > > > 
> > > > > Is getting a slightly LLVM constant actually important here?  I would prefer to avoid this complexity (and unnecessary recursive walk) if possible.
> > > > Since it's target dependent, it won't be constant folded by the target agnostic passes until very late in the backend, which may lose optimization opportunities. I think it's better folded by FE like other constants.
> > > Are you sure?  All of my attempts to produce these constants in LLVM seem to get instantly folded, even without a target set in the IR:
> > >   i32 ptrtoint (i8* inttoptr (i32 -1 to i8*) to i32)
> > >   i64 ptrtoint (i8* inttoptr (i64 -1 to i8*) to i64)
> > > This folding literally occurs within ConstantExpr::getPtrToInt, without any passes running, and it seems to happen regardless of the pointer having an addrspace.
> > > 
> > > I suspect that the problem is that you additionally have an addrspacecast constant in place, which almost certainly does block the constant folder.  The right fix for that is to ensure that your target's implementation of performAddrSpaceCast makes an effort to peephole casts of Constants.
> > In amdgpu target, null pointer in addr space 0 is represented as `addrspacecast int addrspace(4)* null to int*` instead of `inttoptr i32 -1 to i8*`. As addrspacecast is opaque to target-independent LLVM passes, they don't know how to fold it. Only the backend knows how to fold it.
> > 
> > This could be a general situation, i.e., the non-zero null pointer is represented in some target specific form which LLVM does not know how to fold.
> LLVM has deeply-baked-in assumptions that null is a zero bit pattern.  It is really, really not a good idea to assume that LLVM will never make any assumptions about the behavior of addrspacecasts in its target-independent code.
> 
> Also, it is almost certainly possible to validly produce that constant with the expectation that it will have value 0, and thus you special-case lowering in the backend would be a miscompile.
> 
> I understand that you probably got into this situation by trying to give null a non-zero representation in the backend and then slowly realized that you needed frontend changes to avoid miscompiles.  That is totally understandable.  Please understand that what are you actually doing now with this patch is fundamentally changing where you lower null pointers, so that it's now only the frontend which knows about the representation of null pointers, and the backend just treats ConstantPointerNull as a special way of spelling the valid, semantically non-null zero representation of a pointer.
Thanks John. I think your concern is valid. Actually I have fixed some issues in LLVM about incorrect assumptions about addrspacecast'ed bits. Personally I prefer using `inttoptr -1` instead of `addrspacecast null` to represent non-zero null pointer, but I need to discuss with my team before making the change.

For now, I will drop the above code for constant folding of ptrtoint of non-zero null pointer.


https://reviews.llvm.org/D26196