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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
