rsmith added inline comments.
================ Comment at: clang/lib/CodeGen/CGCall.cpp:2165 + assert(getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0 && + "Expected `this` pointer without address space attribute."); + ---------------- jdoerfert wrote: > I'm unsure why this assertion has to hold and more importantly why we need > it. @arsenm do you? Even if the `this` pointer is always an address-space-0 pointer for now, it would be more consistent with how we handle `Attribute::NonNull` elsewhere to skip adding the `NonNull` attribute for non-address-space-0 pointers rather than asserting here. ================ Comment at: clang/lib/CodeGen/CGCall.cpp:2174 + } + unsigned ArgNo = 0; ---------------- jdoerfert wrote: > rsmith wrote: > > CJ-Johnson wrote: > > > jdoerfert wrote: > > > > Even if null pointer is valid we should place dereferenceable. > > > > > > > > We also could never place nonnull and let the middle-end make the > > > > dereferenceable -> nonnull deduction, though I don't see why we can't > > > > just add nonnull here. > > > I re-ran ninja check after making this fix and addressed the > > > newly-affected tests. So the patch is fully up to date :) > > The LLVM LangRef says that in address space 0, `dereferenceable` implies > > `nonnull`, so I don't think we can emit `dereferenceable` in > > `NullPointerIsValid` mode, and we'd need to use `dereferenceable_or_null` > > instead. (Perhaps the LangRef is wrong, though, and the > > `null_pointer_is_valid` function attribute overrides that determination.) > > (Perhaps the LangRef is wrong, though, and the null_pointer_is_valid > > function attribute overrides that determination.) > > This is the case. IMHO, dereferenceable has to be correct here, regardless of > the mode. You could access the object in the function entry, which we would > use to deduce dereferenceable, and if the `NullPointerIsValid` is not > translated to the function attribute also to `nonnull`. OK, if the LangRef is wrong about this, then I agree we should emit `dereferenceable` unconditionally. Thanks for clarifying. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D17993/new/ https://reviews.llvm.org/D17993 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
