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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:306
+  /// \param AddrSpace address space of pointee in source language.
+  virtual uint64_t getNullPtrValue(unsigned AddrSpace) const {
+    return 0;
----------------
We normally spell out "Pointer", I think.


================
Comment at: lib/AST/APValue.cpp:585
+bool APValue::isNullPtr() const {
+  return isLValue() && ((const LV*)(const char*)Data.buffer)->IsNullPtr;
+}
----------------
Seems reasonable to assert isLValue() here.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:1054
+    return ICE->getCastKind() == CK_NullToPointer &&
+        CGF.getTypes().isZeroInitializable(E->getType());
   // '\0'
----------------
This is probably a common enough case that it's worth "inlining", i.e. adding a isPointerZeroInitializable() method that asserts that it's been given a pointer type and does the right thing with it.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1407
     return Builder.CreatePointerBitCastOrAddrSpaceCast(Src,
                                                        ConvertType(DestTy));
   }
----------------
As discussed, please also delegate addrspace conversions into TargetCodeGenInfo.  You don't have to implement them for your target if you don't want, but please do the delegation correctly.

Also, even if you decide not to implement the actual dynamic comparison logic, you should at least teach your target's implementation to recognize when the original pointer is a constant null pointer and just produce the correct new constant.  That should eliminate the need for this special case here, as well as fixing the case where E is a null pointer with side effects like (foo(), NULL).


================
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");
----------------
Why is this necessary?  ptrtoint on the recursively-emitted null pointer should do this automatically.


================
Comment at: lib/CodeGen/CodeGenModule.h:1156
+  /// Get target specific null pointer.
+  llvm::Constant *getNullPtr(llvm::PointerType *T, QualType QT);
+
----------------
Like above, this should be getNullPointer.  Also, please document which type QT is: the pointer type or the pointee type.


================
Comment at: lib/CodeGen/TargetInfo.h:228
+  virtual llvm::Constant *getNullPtr(const CodeGen::CodeGenModule &CGM,
+      llvm::PointerType *T, QualType QT) const;
+
----------------
Same as the comment on getNullPtr above.


https://reviews.llvm.org/D26196