[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: clang/lib/CodeGen/Address.h:67
       return;
-    // Currently the max supported alignment is much less than 1 << 63 and is
+    // Currently the max supported alignment is much less than 1 << 32 and is
     // guaranteed to be a power of 2, so we can store the log of the alignment
----------------
ahatanak wrote:
> efriedma wrote:
> > This comment isn't right.  The max alignment is, as far as I can tell, 1<<32 exactly.  (But there's something weird going on with very large values... somehow `int a[1LL<<32] __attribute((aligned(1ULL<<32))) = {};` ignores the alignment.)
> The following function generated by tablegen (and a few others directly or indirectly calling the function) returns a 32-bit int, but it should be returning a 64-bit int.
> 
> https://github.com/llvm/llvm-project/blob/main/clang/utils/TableGen/ClangAttrEmitter.cpp#L532
Filed https://github.com/llvm/llvm-project/issues/60752 so we don't lose track of this.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:1723
+      Addr =
+          Addr.withPointer(Builder.CreateThreadLocalAddress(GV), KnownNonNull);
 
----------------
ahatanak wrote:
> ahatanak wrote:
> > `KnownNonNull` is being passed here because the address of a load must be non-null. Do we have to check that the function doesn't have `null_pointer_is_valid` here?
> I don't think we should pass `KnownNonNull` unless we know for sure the address of the global variable is non-null, which isn't always true (for example, when it's extern weak).
> 
> We can set `KnownNonNull` below this line if we know `NullPointerIsValid` is false.
If you're assuming something is non-null based on the fact that loads from null trap, then yes, you need to check NullPointerIsValid.

If you want to see the current logic LLVM uses to figure out whether a global can be null, see evaluateICmpRelation in llvm/lib/IR/ConstantFold.cpp.  But in this context, you probably don't need that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142584/new/

https://reviews.llvm.org/D142584