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